Merge #143096

143096: kv: remove ShouldWinOriginTimestampTie option fromcput r=jeffswenson a=jeffswenson

The current KV writer does not use ShouldWinOriginTimestampTie, so it can be removed as unused code. The KV LDR writer stopped using this approach for tie-breaking due to two issues:

1. When replaying replicated writes, they win LWW against themselves, leading to duplicate KVs. This is inefficient, slowing performance and wasting storage until garbage collection (GC) removes the duplicates.
2. The approach fails to handle semantics correctly in three-way replication scenarios.

Release note: none
Epic: CRDB-48647

Co-authored-by: Jeff Swenson <[email protected]>

Author: craig[bot]

pkg/kv/batch.go

@@ -578,9 +578,7 @@ func (b *Batch) CPutAllowingIfNotExists(key, value interface{}, expValue []byte)
 // This is used by logical data replication and other uses of this API
 // are discouraged since the semantics are subject to change as
 // required by that feature.
-func (b *Batch) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
-) {
+func (b *Batch) CPutWithOriginTimestamp(key, value interface{}, expValue []byte, ts hlc.Timestamp) {
 	k, err := marshalKey(key)
 	if err != nil {
 		b.initResult(0, 1, notRaw, err)
@@ -594,7 +592,6 @@ func (b *Batch) CPutWithOriginTimestamp(
 	}
 	r := kvpb.NewConditionalPut(k, v, expValue, false)
 	r.(*kvpb.ConditionalPutRequest).OriginTimestamp = ts
-	r.(*kvpb.ConditionalPutRequest).ShouldWinOriginTimestampTie = shouldWinTie
 	b.appendReqs(r)
 	b.approxMutationReqBytes += len(k) + len(v.RawBytes)
 	b.initResult(1, 1, notRaw, nil)

pkg/kv/kvpb/api.proto

@@ -372,13 +372,7 @@ message ConditionalPutRequest {
   // Used by logical data replication.
   util.hlc.Timestamp origin_timestamp = 8 [(gogoproto.nullable) = false];
 
-  // ShouldWinOriginTimestampTie, if true, indicates that if the "comparison
-  // timestamp" (see comment on OriginTimestamp) and OriginTimestamp are equal,
-  // then the ConditionalPut should succeed (assuming the expected and actual
-  // bytes also match).
-  //
-  // This must only be used in conjunction with OriginTimestamp.
-  bool should_win_origin_timestamp_tie = 9;
+  reserved 9;
 }
 
 // A ConditionalPutResponse is the return value from the

pkg/kv/kvserver/batcheval/cmd_conditional_put.go

@@ -79,9 +79,8 @@ func ConditionalPut(
 			TargetLockConflictBytes:        storage.TargetBytesPerLockConflictError.Get(&cArgs.EvalCtx.ClusterSettings().SV),
 			Category:                       fs.BatchEvalReadCategory,
 		},
-		AllowIfDoesNotExist:         storage.CPutMissingBehavior(args.AllowIfDoesNotExist),
-		OriginTimestamp:             args.OriginTimestamp,
-		ShouldWinOriginTimestampTie: args.ShouldWinOriginTimestampTie,
+		AllowIfDoesNotExist: storage.CPutMissingBehavior(args.AllowIfDoesNotExist),
+		OriginTimestamp:     args.OriginTimestamp,
 	}
 
 	var err error

pkg/sql/colenc/bench_test.go

@@ -138,7 +138,7 @@ type noopPutter struct{}
 
 func (n *noopPutter) CPut(key, value interface{}, expValue []byte) {}
 func (n *noopPutter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, ts hlc.Timestamp,
 ) {
 }
 func (n *noopPutter) Put(key, value interface{})                                {}

pkg/sql/colenc/encode_test.go

@@ -709,7 +709,7 @@ func (c *capturePutter) CPut(key, value interface{}, expValue []byte) {
 }
 
 func (c *capturePutter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, ts hlc.Timestamp,
 ) {
 	colexecerror.InternalError(errors.New("unimplemented"))
 }

pkg/sql/row/helper.go

@@ -528,7 +528,6 @@ func (rh *RowHelper) deleteIndexEntry(
 // option set.
 type OriginTimestampCPutHelper struct {
 	OriginTimestamp hlc.Timestamp
-	ShouldWinTie    bool
 	// PreviousWasDeleted is used to indicate that the expected
 	// value is non-existent. This is helpful in Deleter to
 	// distinguish between a delete of a value that had no columns
@@ -551,7 +550,7 @@ func (oh *OriginTimestampCPutHelper) CPutFn(
 	if traceKV {
 		log.VEventfDepth(ctx, 1, 2, "CPutWithOriginTimestamp %s -> %s @ %s", *key, value.PrettyPrint(), oh.OriginTimestamp)
 	}
-	b.CPutWithOriginTimestamp(key, value, expVal, oh.OriginTimestamp, oh.ShouldWinTie)
+	b.CPutWithOriginTimestamp(key, value, expVal, oh.OriginTimestamp)
 }
 
 func (oh *OriginTimestampCPutHelper) DelWithCPut(
@@ -560,7 +559,7 @@ func (oh *OriginTimestampCPutHelper) DelWithCPut(
 	if traceKV {
 		log.VEventfDepth(ctx, 1, 2, "CPutWithOriginTimestamp %s -> nil (delete) @ %s", key, oh.OriginTimestamp)
 	}
-	b.CPutWithOriginTimestamp(key, nil, expVal, oh.OriginTimestamp, oh.ShouldWinTie)
+	b.CPutWithOriginTimestamp(key, nil, expVal, oh.OriginTimestamp)
 }
 
 func FetchSpecRequiresRawMVCCValues(spec fetchpb.IndexFetchSpec) bool {

pkg/sql/row/putter.go

@@ -20,7 +20,7 @@ import (
 // encoding logic to kv.Batch.
 type Putter interface {
 	CPut(key, value interface{}, expValue []byte)
-	CPutWithOriginTimestamp(key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool)
+	CPutWithOriginTimestamp(key, value interface{}, expValue []byte, ts hlc.Timestamp)
 	Put(key, value interface{})
 	PutMustAcquireExclusiveLock(key, value interface{})
 	Del(key ...interface{})
@@ -47,10 +47,10 @@ func (t *TracePutter) CPut(key, value interface{}, expValue []byte) {
 }
 
 func (t *TracePutter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, ts hlc.Timestamp,
 ) {
 	log.VEventfDepth(t.Ctx, 1, 2, "CPutWithOriginTimestamp %v -> %v @ %v", key, value, ts)
-	t.Putter.CPutWithOriginTimestamp(key, value, expValue, ts, shouldWinTie)
+	t.Putter.CPutWithOriginTimestamp(key, value, expValue, ts)
 }
 
 func (t *TracePutter) Put(key, value interface{}) {
@@ -182,9 +182,9 @@ func (s *SortingPutter) CPut(key, value interface{}, expValue []byte) {
 }
 
 func (s *SortingPutter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, ts hlc.Timestamp,
 ) {
-	s.Putter.CPutWithOriginTimestamp(key, value, expValue, ts, shouldWinTie)
+	s.Putter.CPutWithOriginTimestamp(key, value, expValue, ts)
 }
 
 func (s *SortingPutter) Put(key, value interface{}) {
@@ -279,9 +279,9 @@ type KVBatchAdapter struct {
 var _ Putter = &KVBatchAdapter{}
 
 func (k *KVBatchAdapter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, originTimestamp hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, originTimestamp hlc.Timestamp,
 ) {
-	k.Batch.CPutWithOriginTimestamp(key, value, expValue, originTimestamp, shouldWinTie)
+	k.Batch.CPutWithOriginTimestamp(key, value, expValue, originTimestamp)
 }
 
 func (k *KVBatchAdapter) CPut(key, value interface{}, expValue []byte) {

pkg/sql/row/row_converter.go

@@ -76,7 +76,7 @@ func (c KVInserter) PutMustAcquireExclusiveLock(key, value interface{}) {
 	panic(errors.AssertionFailedf("unimplemented"))
 }
 func (c KVInserter) CPutWithOriginTimestamp(
-	key, value interface{}, expValue []byte, ts hlc.Timestamp, shouldWinTie bool,
+	key, value interface{}, expValue []byte, ts hlc.Timestamp,
 ) {
 	panic(errors.AssertionFailedf("unimplemented"))
 }

pkg/storage/mvcc.go

@@ -2899,13 +2899,6 @@ type ConditionalPutWriteOptions struct {
 	// See the comment on the OriginTimestamp field of
 	// kvpb.ConditionalPutRequest for more details.
 	OriginTimestamp hlc.Timestamp
-	// ShouldWinOriginTimestampTie indicates whether the value should be
-	// accepted if the origin timestamp is the same as the
-	// origin_timestamp/mvcc_timestamp of the existing value.
-	//
-	// See the comment on the ShouldWinOriginTimestampTie field of
-	// kvpb.ConditionalPutRequest for more details.
-	ShouldWinOriginTimestampTie bool
 }
 
 // MVCCConditionalPut sets the value for a specified key only if the expected
@@ -3035,8 +3028,7 @@ func mvccConditionalPutUsingIter(
 		}
 	} else {
 		valueFn = func(existVal optionalValue) (roachpb.Value, error) {
-			originTSWinner, existTS := existVal.isOriginTimestampWinner(opts.OriginTimestamp,
-				opts.ShouldWinOriginTimestampTie)
+			originTSWinner, existTS := existVal.isOriginTimestampWinner(opts.OriginTimestamp, false)
 			if !originTSWinner {
 				return roachpb.Value{}, &kvpb.ConditionFailedError{
 					OriginTimestampOlderThan: existTS,