kv/kvserver: allow the merge transaction to be pushed

[ajwerner]

Historically we have not allowed the merge transaction to be pushed. The
reason we disabled this was because of the hazard due to attempting to
refresh reads on the RHS of the merge after the SubsumeRequest has been
sent. The SubsumeRequest effectively freezes the RHS until the merge
commits or aborts. In order to side-step this hazard, this change ensures
that nothing should prevent the merge transaction from either committing
or aborting.

kv/kvclient: add ManualRefresh support for transactions

This commit adds support for client-initiated refreshes of transactions.
The implementation is somewhat simplistic in that it hijacks existing
logic that occurs during the sending of a request. This makes the
implementation more uniform with the rest of the client library at
the cost of being somewhat awkward and implicit from a code-reading
perspective.

The motivation for this change is to provide the necessary tools to allow
the merge transaction to get pushed. The adoption follows in the next
commit.

Fixes #59308.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

This probably deserves some more advanced testing involving pushing the merge transaction after it has sent the SubsumeRequest.

[nvanbenschoten]

Thanks!

This probably deserves some more advanced testing involving pushing the merge transaction after it has sent the SubsumeRequest.

I'm going to be adding a test in to make sure that two ranges with global_reads: true can be merged, which will force the merge txn to get pushed when writing to local descriptors, so feel free to hold off on this test if you aren't feeling inspired to add it now.

Reviewed 6 of 6 files at r1, 1 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

---

pkg/kv/sender.go, line 294 at r1 (raw file):

	GetSteppingMode(ctx context.Context) (curMode SteppingMode)

	// ForceRefreshTransaction attempts to refresh a transactions read timestamp

nit: same question about naming. Consider renaming to ManualRefresh and moving this next to ManualRestart.

---

pkg/kv/txn.go, line 1299 at r1 (raw file):

}

// ForceRefresh forces a refresh of the read timestamp of a transaction to

nit: did you consider the name ManualRefresh to parallel the existing ManualRestart?

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go, line 2454 at r1 (raw file):

				}
				require.NoError(t, <-errCh)
				require.NoError(t, txn.ForceRefresh(ctx))

Comment that the fact that this doesn't block on the testing machinery indicates that it's a no-op.

---

pkg/kv/kvserver/client_merge_test.go, line 1325 at r2 (raw file):

	}
	if mergePostSplitTS.LessEq(splitTS) {
		t.Fatalf("expected merge to start before concurrent split, %v <= %v", mergePostSplitTS, splitTS)

s/start before/finish after/?

[ajwerner]

so feel free to hold off on this test if you aren't feeling inspired to add it now.

Excellent

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

---

pkg/kv/sender.go, line 294 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: same question about naming. Consider renaming to ManualRefresh and moving this next to ManualRestart.

Done.

---

pkg/kv/txn.go, line 1299 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: did you consider the name ManualRefresh to parallel the existing ManualRestart?

Done.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go, line 2454 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Comment that the fact that this doesn't block on the testing machinery indicates that it's a no-op.

Done.

---

pkg/kv/kvserver/client_merge_test.go, line 1325 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/start before/finish after/?

Done.

[ajwerner]

TFTR!

bors r=nvanbenschoten

[craig]

Build succeeded:

• GitHub CI (Cockroach)