storage: Splits get write locks on the entire span

pkg/storage/replica_command.go

@@ -563,17 +563,20 @@ func declareKeysEndTransaction(
 
 	if et.InternalCommitTrigger != nil {
 		if st := et.InternalCommitTrigger.SplitTrigger; st != nil {
-			// Splits may read from the entire pre-split range and write to
-			// the right side's RangeID spans and abort cache.
-			// TODO(bdarnell): the only time we read from the right-hand
-			// side is when the existing stats contain estimates. We might
-			// be able to be smarter here and avoid declaring reads on RHS
-			// in most cases.
-			spans.Add(SpanReadOnly, roachpb.Span{
+			// Splits may read from the entire pre-split range (they read
+			// from the LHS in all cases, and the RHS only when the existing
+			// stats contain estimates), but they need to declare a write
+			// access to block all other concurrent writes. We block writes
+			// to the RHS because they will fail if applied after the split,
+			// and writes to the LHS because their stat deltas will
+			// interfere with the non-delta stats computed as a part of the
+			// split. (see
+			// https://github.com/cockroachdb/cockroach/issues/14881)
+			spans.Add(SpanReadWrite, roachpb.Span{
 				Key:    st.LeftDesc.StartKey.AsRawKey(),
 				EndKey: st.RightDesc.EndKey.AsRawKey(),
 			})
-			spans.Add(SpanReadOnly, roachpb.Span{
+			spans.Add(SpanReadWrite, roachpb.Span{
 				Key:    keys.MakeRangeKeyPrefix(st.LeftDesc.StartKey),
 				EndKey: keys.MakeRangeKeyPrefix(st.RightDesc.EndKey).PrefixEnd(),
 			})

pkg/storage/replica_test.go

@@ -2451,6 +2451,39 @@ func TestReplicaCommandQueueTimestampNonInterference(t *testing.T) {
 	}
 }
 
+// TestReplicaCommandQueueSplitDeclaresWrites verifies that split
+// operations declare write access to their entire span. This is
+// necessary to avoid conflicting changes to the range's stats, even
+// though splits do not actually write to their data span (and
+// therefore a failure to declare writes are not caught directly by
+// any other test).
+func TestReplicaCommandQueueSplitDeclaresWrites(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	var spans SpanSet
+	commands[roachpb.EndTransaction].DeclareKeys(
+		roachpb.RangeDescriptor{StartKey: roachpb.RKey("a"), EndKey: roachpb.RKey("d")},
+		roachpb.Header{},
+		&roachpb.EndTransactionRequest{
+			InternalCommitTrigger: &roachpb.InternalCommitTrigger{
+				SplitTrigger: &roachpb.SplitTrigger{
+					LeftDesc: roachpb.RangeDescriptor{
+						StartKey: roachpb.RKey("a"),
+						EndKey:   roachpb.RKey("c"),
+					},
+					RightDesc: roachpb.RangeDescriptor{
+						StartKey: roachpb.RKey("c"),
+						EndKey:   roachpb.RKey("d"),
+					},
+				},
+			},
+		},
+		&spans)
+	if err := spans.checkAllowed(SpanReadWrite, roachpb.Span{Key: roachpb.Key("b")}); err != nil {
+		t.Fatalf("expected declaration of write access, err=%s", err)
+	}
+}
+
 func SendWrapped(
 	ctx context.Context, sender client.Sender, header roachpb.Header, args roachpb.Request,
 ) (roachpb.Response, roachpb.BatchResponse_Header, *roachpb.Error) {