kv: bump timestamp cache when releasing replicated locks synchronously

[nvanbenschoten]

Fixes #111536.

This commit is the second half of #111546. Whereas that commit taught intent/lock resolution to bump the timestamp cache when releasing replicated locks for committed transactions asynchronously through ResolveIntent and ResolveIntentRange requests, this commit teaches intent/lock resolution to bump the timestamp cache when releasing replicated locks for committed transactions synchronously through EndTxn requests.

The interface changes here look slightly different than the ones in that commit. Instead of attaching the commit timestamp to the response proto, we attach the spans over which replicated locks were released. This is because the commit timestamp was already present on the response, but the request may have resolved multiple lock spans.

Release note: None

[cockroach-teamcity]

This change is 

[miraradeva]

Just a few nits and one question.

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

---

pkg/kv/kvpb/api.proto line 985 at r1 (raw file):

  // spans when committing this transaction. Notably, this field is left unset
  // if only write intents were resolved. The field is also left unset for
  // transactions who aborted.

nit: "transactions that aborted."

---

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

			// the released locks continue to provide protection against writes
			// underneath the transaction's commit timestamp.
			for _, sp := range resp.(*kvpb.EndTxnResponse).ReplicatedLocksReleasedOnCommit {

If a txn committed successfully, is it guaranteed to have released/resolved all locks in its LockSpans? If so, what would happen if here we iterated over req.(*kvpb.EndTxnRequest).LockSpans instead of the actually-released replicated locks?

I guess one difference from the current logic is that we'll also bump the ts cache for spans that include only write intents. What would that do (other than being unnecessary)?

Is it possible that different (e.g. more) locks were released than were included in LockSpans?

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 634 at r1 (raw file):

			}
			if replLocksReleased {
				releasedReplLocks = append(releasedReplLocks, update.Span)

nit: replLocksReleased and releasedReplLocks is a bit confusing. I think releasedReplLocks is a good name for the slice; for the boolean, maybe wereReplLocksReleased or something shorter along those lines?

---

pkg/kv/kvserver/replica_test.go line 14618 at r1 (raw file):

		et, _ := endTxnArgs(txn, isCommit)
		// Assign lock spans to the EndTxn request. Assign one point lock span
		// and one range lock spans.

nit: "and one range lock span."

[nvanbenschoten]

TFTR!

bors r=miraradeva

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

---

pkg/kv/kvpb/api.proto line 985 at r1 (raw file):

Previously, miraradeva (Mira Radeva) wrote…

nit: "transactions that aborted."

Done.

---

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

If a txn committed successfully, is it guaranteed to have released/resolved all locks in its LockSpans?

No, not all of the LockSpans that were included in the request must have been resolved/released by this point. Those that were not resolved are written into the transaction record for asynchronous resolution. The relevant logic is in cmd_end_transaction.go, with the handling of the externalLocks variable.

There are two reasons why a span in the EndTxn request may not be resolved by this point:

1. it references keys on a different range than that in which the transaction record is stored.
2. synchronous resolution (resolveLocalLocks) hit a key or byte limit and left the remaining locks on the transaction record's range for asynchronous resolution.

I guess one difference from the current logic is that we'll also bump the ts cache for spans that include only write intents. What would that do (other than being unnecessary)?

It wouldn't be incorrect, but it would be unnecessary and put undue pressure on the timestamp cache's memory limit, which may lead to the timestamp cache losing fidelity faster than it otherwise would.

Is it possible that different (e.g. more) locks were released than were included in LockSpans?

It's not possible that more locks were released than were included in LockSpans, only that fewer were.

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 634 at r1 (raw file):

Previously, miraradeva (Mira Radeva) wrote…

nit: replLocksReleased and releasedReplLocks is a bit confusing. I think releasedReplLocks is a good name for the slice; for the boolean, maybe wereReplLocksReleased or something shorter along those lines?

Done.

---

pkg/kv/kvserver/replica_test.go line 14618 at r1 (raw file):

Previously, miraradeva (Mira Radeva) wrote…

nit: "and one range lock span."

Done.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)