kvclient: DeleteRequest's reads from write buffer

In order to ensure that a DELETE returns the correct number of deleted
rows, it must read from the write buffer so that it observes any
previous deletes or writes.

Note that all DELETE statements are not yet handled since we don't yet
have DeleteRange support.

Epic: none
Release note: None

Author: stevendanna

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go

@@ -396,6 +396,15 @@ func (twb *txnWriteBuffer) applyTransformations(
 			twb.addToBuffer(t.Key, t.Value, t.Sequence)
 
 		case *kvpb.DeleteRequest:
+			// To correctly populate FoundKey in the response, we need to look in our
+			// write buffer to see if there is a tombstone.
+			var foundKey bool
+			val, served := twb.maybeServeRead(t.Key, t.Sequence)
+			if served {
+				log.VEventf(ctx, 2, "serving read portion of %s on key %s from the buffer", t.Method(), t.Key)
+				foundKey = val.IsPresent()
+			}
+
 			// If MustAcquireExclusiveLock flag is set on the DeleteRequest, then we
 			// need to add a locking Get to the BatchRequest, including if the key
 			// doesn't exist.
@@ -414,10 +423,20 @@ func (twb *txnWriteBuffer) applyTransformations(
 				baRemote.Requests = append(baRemote.Requests, getReqU)
 			}
 
+			// If we found a key in our write buffer we use that
+			// result regardless of what the GetResponse that we
+			// might have sent says.
+			//
+			// NOTE(ssd): We are assuming that callers who care
+			// about an accurate value of FoundKey also set
+			// MustAcquireExclusiveLock.
 			var ru kvpb.ResponseUnion
-			ru.MustSetInner(&kvpb.DeleteResponse{
-				FoundKey: false,
-			})
+			if served || !t.MustAcquireExclusiveLock {
+				ru.MustSetInner(&kvpb.DeleteResponse{
+					FoundKey: foundKey,
+				})
+			}
+
 			ts = append(ts, transformation{
 				stripped:    !t.MustAcquireExclusiveLock,
 				index:       i,
@@ -812,13 +831,19 @@ func (t transformation) toResp(
 		ru = t.resp
 
 	case *kvpb.DeleteRequest:
-		getResp := br.GetInner().(*kvpb.GetResponse)
 		ru = t.resp
-		if log.ExpensiveLogEnabled(ctx, 2) {
-			log.Eventf(ctx, "synthesizing DeleteResponse from GetResponse: %#v", getResp)
+		// If the deletion response is already set, it means we served response from
+		// the write buffer. We can still be here because we happened to need to
+		// send a GetRequest solely for the locking behaviour.
+		if ru.GetDelete() == nil {
+			getResp := br.GetInner().(*kvpb.GetResponse)
+			if log.ExpensiveLogEnabled(ctx, 2) {
+				log.Eventf(ctx, "synthesizing DeleteResponse from GetResponse: %#v", getResp)
+			}
+			ru.MustSetInner(&kvpb.DeleteResponse{
+				FoundKey: getResp.Value.IsPresent(),
+			})
 		}
-		ru.GetDelete().FoundKey = getResp.Value.IsPresent()
-
 	case *kvpb.GetRequest:
 		// Get requests must be served from the local buffer if a transaction
 		// performed a previous write to the key being read. However, Get requests

pkg/kv/kvpb/api.proto

@@ -408,7 +408,11 @@ message DeleteRequest {
   RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
   // MustAcquireExclusiveLock, if set, indicates that a lock with the strength
   // no lower than "exclusive" needs to be acquired on the key, even if it
-  // doesn't exist.
+  // doesn't exist. Note that when MustAcquireExclusiveLock is false, the
+  // FoundKey field in the DeleteResponse may be incorrect when the kvclient has
+  // been configured to buffer writes.
+  //
+  // TODO(ssd): Separate the behaviour of FoundKey to not depend on this flag.
   bool must_acquire_exclusive_lock = 2;
 }
 

pkg/sql/logictest/logic.go

@@ -2086,6 +2086,21 @@ func (c clusterOptIgnoreStrictGCForTenants) apply(args *base.TestServerArgs) {
 	args.Knobs.Store.(*kvserver.StoreTestingKnobs).IgnoreStrictGCEnforcement = true
 }
 
+// clusterOptDisableUseMVCCRangeTombstonesForPointDeletes corresponds
+// to the disable-mvcc-range-tombstones-for-point-deletes directive.
+type clusterOptDisableUseMVCCRangeTombstonesForPointDeletes struct{}
+
+var _ clusterOpt = clusterOptDisableUseMVCCRangeTombstonesForPointDeletes{}
+
+// apply implements the clusterOpt interface.
+func (c clusterOptDisableUseMVCCRangeTombstonesForPointDeletes) apply(args *base.TestServerArgs) {
+	_, ok := args.Knobs.Store.(*kvserver.StoreTestingKnobs)
+	if !ok {
+		args.Knobs.Store = &kvserver.StoreTestingKnobs{}
+	}
+	args.Knobs.Store.(*kvserver.StoreTestingKnobs).EvalKnobs.UseRangeTombstonesForPointDeletes = false
+}
+
 // knobOptDisableCorpusGeneration disables corpus generation for declarative
 // schema changer.
 type knobOptDisableCorpusGeneration struct{}
@@ -2230,6 +2245,8 @@ func readClusterOptions(t *testing.T, path string) []clusterOpt {
 			res = append(res, clusterOptTracingOff{})
 		case "ignore-tenant-strict-gc-enforcement":
 			res = append(res, clusterOptIgnoreStrictGCForTenants{})
+		case "disable-mvcc-range-tombstones-for-point-deletes":
+			res = append(res, clusterOptDisableUseMVCCRangeTombstonesForPointDeletes{})
 		default:
 			t.Fatalf("unrecognized cluster option: %s", opt)
 		}

pkg/sql/logictest/testdata/logic_test/buffered_writes

@@ -1,11 +1,12 @@
-
-subtest point_delete
+# cluster-opt: disable-mvcc-range-tombstones-for-point-deletes
 
 statement ok
 SET kv_transaction_buffered_writes_enabled=true
 
 statement ok
-CREATE TABLE t1 (pk int primary key, v int)
+CREATE TABLE t1 (pk int primary key, v int, FAMILY (pk, v))
+
+subtest point_delete
 
 statement ok
 INSERT INTO t1 VALUES (1,1)
@@ -21,3 +22,41 @@ DELETE FROM t1 WHERE pk = 3
 
 statement ok
 COMMIT
+
+subtest repeated_point_delete
+
+statement ok
+INSERT INTO t1 VALUES (1,1)
+
+statement ok
+BEGIN
+
+statement count 1
+DELETE FROM t1 WHERE pk = 1
+
+# The second delete should be served from the write buffer and observe
+# the buffered tombstone.
+statement count 0
+DELETE FROM t1 WHERE pk = 1
+
+statement ok
+COMMIT
+
+subtest point_delete_after_write
+
+statement ok
+BEGIN
+
+statement ok
+INSERT INTO t1 VALUES (1,1)
+
+statement count 1
+DELETE FROM t1 WHERE pk = 1
+
+# The second delete should be served from the write buffer and observe
+# the buffered tombstone.
+statement count 0
+DELETE FROM t1 WHERE pk = 1
+
+statement ok
+COMMIT