kv: allow DeleteRangeRequests to be pipelined #119975

arulajmani · 2024-03-05T23:48:32Z

Previously, ranged requests could not be pipelined. However, there is no
good reason to not allow them to be pipeliend -- we just have to take
extra care to correctly update in-flight writes tracking on the response
path. We do so now.

As part of this patch, we introduce two new flags -- canPipeline and
canParallelCommit. We use these flags to determine whether batches can
be pipelined or committed using parallel commits. This is in contrast to
before, where we derived this information from other flags
(isIntentWrite, !isRange). This wasn't strictly necessary for this
change, but helps clean up the concepts.

As a consequence of this change, we now have a distinction between
requests that can be pipelined and requests that can be part of a batch
that can be committed in parallel. Notably, this applies to
DeleteRangeRequests -- they can be pipeliend, but not be committed in
parallel. That's because we need to have the entire write set upfront
when performing a parallel commit, lest we need to perform recovery --
we don't have this for DeleteRange requests.

In the future, we'll extend the concept of canPipeline
(and !canParallelCommit) to other locking ranged requests as well. In
particular, (replicated) locking {,Reverse}ScanRequests who want to
pipeline their lock acquisitions.

Closes #64723
Informs #117978

Release note: None

cockroach-teamcity · 2024-03-05T23:48:42Z

This change is

nvanbenschoten

This is looking good! Have you tested it out end-to-end with SQL in an MR cluster to show the latency win? You can use the script from #64723.

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

pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go line 139 at r2 (raw file):

	mu                 sync.Locker
	disable1PC         bool
	disableElideEndTxn bool

I don't understand why this part of the change was needed. Maintaining a separate flag instead of deriving this from the EndTxn request feels like a step backwards. Doing so means that we need to keep the flag in sync with the rest of the txn, lest we accidentally disable elision too much or too little.

For example, on first reading, it looks like there's a bug here where we should reset this flag to false in epochBumpedLocked. That's not actually a bug because we don't want to elide in these cases, but the flag being separate from lockFootprint (which documents this) raises these questions.

pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go line 370 at r2 (raw file):

		req := ru.GetInner()
		switch {
		case kvpb.IsIntentWrite(req):

Should we use CanParallelCommit here?

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 462 at r2 (raw file):

		req := ru.GetInner()

		// The current request cannot be pipelined, so it prevents us from

This comment should either start with "Determine whether ..." (with a few other updates) or should be moved within the if block.

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 485 at r2 (raw file):

		}

		switch req.Method() {

nit: we'll need to move this above the CanPipeline check if we listen to the comment I made in api.go.

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 497 at r2 (raw file):

			// can disable DeleteRange pipelining entirely for requests that set this
			// field to false.
			deleteRangeReq.ReturnKeys = true

We'll need to do this for mixed version clusters, but longer term, should we try to just deprecate this flag and always treat it as set to true? It will be set to true most of the time going forward anyway.

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 497 at r2 (raw file):

			// can disable DeleteRange pipelining entirely for requests that set this
			// field to false.
			deleteRangeReq.ReturnKeys = true

Should we add a test that demonstrates that this flag is set to true? Or add it to TestTxnPipelinerDeleteRangeRequests.

pkg/kv/kvpb/api.go line 87 at r2 (raw file):

	isLocking:         {isTxn},
	isIntentWrite:     {isWrite, isLocking},
	canParallelCommit: {canPipeline},

Does canPipeline depend on isIntentWrite?

pkg/kv/kvpb/api.go line 186 at r2 (raw file):

}

// CanPipeline returns true iff the BatchRequest can be pipelined.

s/BatchRequest/command/

Here and below.

pkg/kv/kvpb/api.go line 193 at r2 (raw file):

// CanParallelCommit returns true iff the BatchRequest can be part of a batch
// that is committed in parallel.
func CanParallelCommit(args Request) bool {

Can we use this in place of kvpb.IsIntentWrite(req) && !kvpb.IsRange(req) (twice) in maybeStripInFlightWrites?

pkg/kv/kvpb/api.go line 1669 at r2 (raw file):

	// recovery we need the entire in-flight write set to plop on the txn record,
	// and we don't have that on the request path if the batch contains a
	// DeleteRange request.

Should we only return canPipeline if drr.ReturnKeys?

pkg/kv/kvclient/kvcoord/txn_interceptor_committer_test.go line 75 at r1 (raw file):

			require.Len(t, ba.Requests, 2)
			require.IsType(t, &kvpb.GetRequest{}, ba.Requests[0].GetInner())
			require.IsType(t, &kvpb.PutRequest{}, ba.Requests[1].GetInner())

ScanRequest

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go line 617 at r2 (raw file):

	require.Equal(t, 3, tp.ifWrites.len())

	// Now, test RefreshRangeRequests, which cannot be pipelined.

RefreshRangeRequest will never go through the txnPipeliner, so I don't think this is worth testing. If you want to test a ranged request, maybe use a ScanRequest?

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go line 637 at r2 (raw file):

	require.Nil(t, pErr)
	require.NotNil(t, br)
	require.Equal(t, 0, tp.ifWrites.len())

How do we end up with 0 in-flight writes here? We didn't clear them or issue QueryIntent request for them.

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go line 972 at r2 (raw file):

}

// TestTxnPipelinerDeleteRangeRequests ensures the txnPipelineer correctly

txnPipeliner

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go line 997 at r2 (raw file):

		br.Txn = ba.Txn
		resp := br.Responses[0].GetInner()
		resp.(*kvpb.DeleteRangeResponse).Keys = []roachpb.Key{keyB}

Since this is the only test where we set DeleteRangeResponse.Keys, let's add a few to demonstrate that all end up in ifWrites and that they all end up with the correct sequence number. See the qiReq1 := ... logic above for inspiration.

pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner_test.go line 1010 at r2 (raw file):

	ba = &kvpb.BatchRequest{}
	ba.Header = kvpb.Header{Txn: &txn}
	ba.Add(&kvpb.DeleteRangeRequest{RequestHeader: kvpb.RequestHeader{Key: keyD, EndKey: keyE}})

Let's have this overlap with some (but not all) of the in-flight intent writes from the first request. That will lead to an interesting order of QueryIntentRequests.

Previously, this test was constructing (and testing) an unrealistic scenario. We should never be eliding EndTxn requests if there is a Put in the batch; but, because we weren't correctly populating lock spans, we ended up asserting that we were eliding the request. We now switch to using a ScanRequest in here instead. Epic: none Release note: None

arulajmani

Still need to look into/fix TestTxnPipelinerRejectAboveBudget, but I addressed all your review comments, beefed up some testing, and I think I have all other tests passing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/kv/kvclient/kvcoord/txn_interceptor_committer.go line 139 at r2 (raw file):