kv: non-transactional batches can be split into sub-batches

[danhhz]

Discussed in #46017 (review)

The following diff on top of #46017 finds a kv.Batch committed via kv.DB that has writes with two different timestamps.

diff --git a/pkg/kv/kvnemesis/generator.go b/pkg/kv/kvnemesis/generator.go
index 6cf485c0b7..b438be4a40 100644
--- a/pkg/kv/kvnemesis/generator.go
+++ b/pkg/kv/kvnemesis/generator.go
@@ -179,7 +179,7 @@ func NewDefaultConfig() GeneratorConfig {
 	// nontransactional batch are disjoint and upgrading to a transactional batch
 	// (see CrossRangeTxnWrapperSender) if they are. roachpb.SpanGroup can be used
 	// to efficiently check this.
-	config.Ops.Batch = BatchOperationConfig{}
+	config.Ops.Batch.Ops.PutExisting = 0
 	return config
 }

  {
    b := &Batch{}
    b.Put(ctx, /Table/50/"c1556caf", v-5) // nil
    b.Get(ctx, /Table/50/"43275686") // (nil, nil)
    b.Put(ctx, /Table/50/"0998fa3b", v-6) // nil
    db0.Run(ctx, b) // nil
  }

committed batch different timestamps: /Table/50/"0998fa3b":1583976139.381769000,0->v-6 /Table/50/"c1556caf":1583976139.381491000,0->v-5

Diagnosis by @nvanbenschoten

we're just straight-up missing a check that non-transactional batches don't get split into sub-batches due to request compatibility rules (as opposed to range boundaries). We'd want something like:

diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go
index 36954d9867..9ff2620ddd 100644
--- a/pkg/kv/kvclient/kvcoord/dist_sender.go
+++ b/pkg/kv/kvclient/kvcoord/dist_sender.go
@@ -692,6 +693,12 @@ func (ds *DistSender) Send(
                splitET = true
        }
        parts := splitBatchAndCheckForRefreshSpans(ba, splitET)
+       if len(parts) > 1 && ba.Txn == nil && ba.IsTransactional() && ba.ReadConsistency == roachpb.CONSISTENT {
+               // If there's no transaction and ba spans ranges, possibly re-run as part of
+               // a transaction for consistency. The case where we don't need to re-run is
+               // if the read consistency is not required.
+               return nil, roachpb.NewError(&roachpb.OpRequiresTxnError{})
+       }
        if len(parts) > 1 && (ba.MaxSpanRequestKeys != 0 || ba.TargetBytes != 0) {
                // We already verified above that the batch contains only scan requests of the same type.
                // Such a batch should never need splitting.

Found by kvnemesis.

Jira issue: CRDB-5106

[tbg]

The snippet above has made it into DistSender,

cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go

Lines 1240 to 1245 in 3ff62c3

	// If there's no transaction and ba spans ranges, possibly re-run as part of
	// a transaction for consistency. The case where we don't need to re-run is
	// if the read consistency is not required.
	if ba.Txn == nil && ba.IsTransactional() && ba.ReadConsistency == roachpb.CONSISTENT {
	return nil, roachpb.NewError(&roachpb.OpRequiresTxnError{})
	}

but I just ran into the issue again. The problem is that once we add DeleteRangeUsingTombstone¹, a batch containing that request will happily be split into two parts, so kvnemesis potentially sees multiple mvcc timestamps for it.

Just pointing this out; this can't really be fixed. But the snippet above is problematic if we were to start mixing DeleteRangeUsingTombstone with, say, a Put. That would result in a batch that claims to be "transactional" and we'd restart in a txn and then fail, since mvcc range deletions are unsupported in the context of a transaction.

So the check is wrong in that sense. ba.IsTransactional() means "contains a request that can be used in a txn". What we need is that all requests can be used in a txn.

Besides this also seems to be a real footgun in the KV API. I think we tend to always assume that batches are atomic, but here it's very clear that they mostly, but not always, are. This subtlety is confusing; ideally batches would opt into being split explicitly, and we would always return an error for batches that don't opt in but could in theory require splitting.

We have an isUnsplittable flag - only used for AddSSTable - which isn't really a great solution in general, since it is a property of the request type, where really it is something different callers may think differently about, or in the worst case not think about at all.

Footnotes

1. https://github.com/cockroachdb/cockroach/issues/76435 ↩

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!