kv: ship high-resolution tscache summaries during lease transfers and range merges #118300

nvanbenschoten · 2024-01-25T04:38:14Z

Fixes #61986.
Fixes #117486.
Unblocks #118000.

This commit uses the new tscache.Cache.Serialize method introduced in #118299 to ship high-resolution summaries of the timestamp cache during lease transfers and ranges merges. In doing so, it eliminates the loss of precision that occurs in an incoming leaseholder's timestamp cache when it receives a lease transfer or range merge.

This loss of precision was a source of transaction retries for three reasons:

txn tombstone marker keys would have their timestamp advanced, leading to TransactionAbortedError with the ABORT_REASON_NEW_LEASE_PREVENTS_TXN reason.
txn push marker keys would have their timestamp advanced, leading to transactions having their commit timestamp pushed, which could lead to TransactionRetryError with the RETRY_SERIALIZABLE reason.
global keys would have their timestamp advanced as if they had been read, also leading to transactions having their commit timestamp pushed if they wrote to those keys, which could also lead to TransactionRetryError with the RETRY_SERIALIZABLE reason.

The first issue here is the most severe, both because it can not be refreshed away and because it affects transactions of all isolation levels.

This commit introduces two new cluster settings to control the maximum size of these timestamp cache read summaries:

kv.lease_transfer_read_summary.local_budget
kv.lease_transfer_read_summary.global_budget

It configures the local keyspace budget to 4MB and the global keyspace budget to 0B. This default configuration should be sufficient to eliminate the first two sources of retries described above. The third has not been observed as a serious issue in practice, so we configure the global budget to 0 so that we can hit a serialization fast-path.

Release note (ops change): Transaction replay protection state is now passed between the outgoing and incoming leaseholder for a range during a lease transfer. This avoids cases where lease transfers can cause transactions to throw TransactionAbortedError(ABORT_REASON_NEW_LEASE_PREVENTS_TXN) errors.

cockroach-teamcity · 2024-01-25T04:38:26Z

This change is

arulajmani

Exciting to see this land!

Reviewed 4 of 4 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

-- commits line 14 at r1:
Glad we're no longer imprecise!

-- commits line 48 at r2:
They're only read during node failure, right? Or are there other cases I'm missing?

pkg/kv/kvserver/replica_tscache.go line 39 at r1 (raw file):

		"budget will result in loss of precision.",
	4<<20, /* 4 MB */
)

Did we not want to make this setting public?

pkg/kv/kvserver/replica_tscache.go line 606 at r1 (raw file):

			lease, _ /* nextLease */ := r.GetLease()
			// Recognize the case where a lease started recently. Lease transfers can
			// bump the ts cache low water mark. However, in newer versions of the

nit: s/newer versions/versions >= 24.1/g

pkg/kv/kvserver/replica_tscache.go line 711 at r1 (raw file):

			// Serialize the key range and then compress to the budget.
			seg = tc.Serialize(ctx, start, end)
			seg.Compress(budget)

We could return a boolean from Compress, indicating whether we needed to actually compress or not, and use that to increment some metric that keeps track of the number of times we've run into budget constraints. Feel free to do this in a followup though.

pkg/kv/kvserver/client_lease_test.go line 685 at r1 (raw file):

	err = txn.Commit(ctx)
	require.Regexp(t, `TransactionAbortedError\(ABORT_REASON_NEW_LEASE_PREVENTS_TXN\)`, err)

Is it worth keeping a version of the test around that does run into an ABORT_REASON_NEW_LEASE_PREVENTS_TXN? Maybe for cases where we've overshot the budget?

nvanbenschoten

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)

-- commits line 14 at r1: