Merge #145096 #146098

145096: storage: always write RANGEKEYDELs in ClearRangeWithHeuristic r=sumeerbhola a=jbowens

Previously, ClearRangeWithHeuristic applied a heuristic that would switch between writing RANGEKEYUNSETs versus RANGEKEYDELs. When multiple range keys overlap, unsetting each individual range key with RANGEKEYUNSET is strictly worse than writing a single RANGEKEYDEL over the same span.

This commit adapts ClearRangeWithHeuristic to write a single RANGEKEYDEL over the cleared span, but only if there are indeed range keys within the span. Setting a broad RANGEKEYDEL increases the likelihood that data can be removed using a delete-only compaction.

Close #144954.
Epic: none
Release note: none

146098: logical: set origin id in tombstone updater r=jeffswenson a=jeffswenson

Previously, the tombstone updater did not set an origin id when updating a tombstone. One critical use of the origin id is its used to filter LDR replication events. Events with an origin id are not replicated because replicating a replication write causes an infinite replication loop.

Fixes: #146008
Release note: none

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

Author: 3 people

pkg/crosscluster/logical/BUILD.bazel

@@ -132,6 +132,7 @@ go_test(
         "sql_row_reader_test.go",
         "sql_row_writer_test.go",
         "table_batch_handler_test.go",
+        "tombstone_updater_test.go",
         "udf_row_processor_test.go",
     ],
     data = glob(["testdata/**"]) + [

pkg/crosscluster/logical/tombstone_updater.go

@@ -97,6 +97,7 @@ func (tu *tombstoneUpdater) updateTombstone(
 			// If updateTombstone is called in a transaction, create and run a batch
 			// in the transaction.
 			batch := txn.KV().NewBatch()
+			batch.Header.WriteOptions = originID1Options
 			if err := tu.addToBatch(ctx, txn.KV(), batch, mvccTimestamp, afterRow); err != nil {
 				return err
 			}
@@ -106,6 +107,7 @@ func (tu *tombstoneUpdater) updateTombstone(
 		// 1pc transaction.
 		return tu.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
 			batch := txn.NewBatch()
+			batch.Header.WriteOptions = originID1Options
 			if err := tu.addToBatch(ctx, txn, batch, mvccTimestamp, afterRow); err != nil {
 				return err
 			}

pkg/crosscluster/logical/tombstone_updater_test.go

@@ -0,0 +1,84 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package logical
+
+import (
+	"context"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
+	"github.com/cockroachdb/cockroach/pkg/sql/isql"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/require"
+)
+
+func TestTombstoneUpdaterSetsOriginID(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	// This is a regression test for a bug in the tombstone updater. The
+	// tombstone updater should always set the origin ID. Previously, it would
+	// not set the origin id which caused logical replication to pick up the
+	// tombstone update as a deletion event. This is undesirable because the
+	// tombstone update case is only used when replicating deletes and if a
+	// replicated write generates an LDR event, it leads to looping.
+
+	// Start server with two databases
+	ctx := context.Background()
+	server, s, runners, dbNames := setupServerWithNumDBs(t, ctx, testClusterBaseClusterArgs, 1, 2)
+	defer server.Stopper().Stop(ctx)
+
+	// Create test table on both databases
+	destRunner := runners[1]
+
+	// Create a tombstone updater
+	desc := desctestutils.TestingGetMutableExistingTableDescriptor(
+		s.DB(), s.Codec(), dbNames[0], "tab")
+	sd := sql.NewInternalSessionData(ctx, s.ClusterSettings(), "" /* opName */)
+	tu := newTombstoneUpdater(s.Codec(), s.DB(), s.LeaseManager().(*lease.Manager), desc.GetID(), sd, s.ClusterSettings())
+	defer tu.ReleaseLeases(ctx)
+
+	// Set up 1 way logical replication. The replication stream is used to ensure
+	// that the tombstone update will not be replicated as a deletion event.
+	var jobID jobspb.JobID
+	destRunner.QueryRow(t,
+		"CREATE LOGICAL REPLICATION STREAM FROM TABLE a.tab ON $1 INTO TABLE b.tab",
+		GetPGURLs(t, s, dbNames)[0].String()).Scan(&jobID)
+
+	// Write the row to the destination
+	destRunner.Exec(t, "INSERT INTO tab VALUES (1, 42)")
+
+	row := tree.Datums{
+		tree.NewDInt(tree.DInt(1)), // k
+		tree.DNull,                 // v (deleted)
+	}
+
+	_, err := tu.updateTombstone(ctx, nil, s.Clock().Now(), row)
+	require.NoError(t, err)
+
+	config := s.ExecutorConfig().(sql.ExecutorConfig)
+	err = sql.DescsTxn(ctx, &config, func(
+		ctx context.Context, txn isql.Txn, descriptors *descs.Collection,
+	) error {
+		_, err = tu.updateTombstone(ctx, txn, s.Clock().Now(), row)
+		return err
+	})
+	require.NoError(t, err)
+
+	// Wait for replication to advance
+	WaitUntilReplicatedTime(t, s.Clock().Now(), destRunner, jobID)
+
+	// Verify the row still exists in the destination
+	destRunner.CheckQueryResults(t, "SELECT pk, payload FROM tab", [][]string{
+		{"1", "42"},
+	})
+}

pkg/kv/kvserver/batcheval/cmd_clear_range.go

@@ -150,9 +150,9 @@ func ClearRange(
 	// If we're writing Pebble range tombstones, use ClearRangeWithHeuristic to
 	// avoid writing tombstones across empty spans -- in particular, across the
 	// range key span, since we expect range keys to be rare.
-	const pointKeyThreshold, rangeKeyThreshold = 2, 2
+	const pointKeyThreshold = 2
 	if err := storage.ClearRangeWithHeuristic(
-		ctx, readWriter, readWriter, from, to, pointKeyThreshold, rangeKeyThreshold,
+		ctx, readWriter, readWriter, from, to, pointKeyThreshold,
 	); err != nil {
 		return result.Result{}, err
 	}

pkg/kv/kvserver/client_merge_test.go

@@ -4023,7 +4023,7 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 			EndKey:   roachpb.RKey(keyEnd),
 		}
 		if err := storage.ClearRangeWithHeuristic(
-			ctx, receivingEng, &sst, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey(), 64, 8,
+			ctx, receivingEng, &sst, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey(), 64,
 		); err != nil {
 			return err
 		}

pkg/kv/kvserver/kvstorage/destroy.go

@@ -31,11 +31,6 @@ const (
 	// perhaps we should fix Pebble to handle large numbers of range tombstones in
 	// an sstable better.
 	ClearRangeThresholdPointKeys = 64
-
-	// ClearRangeThresholdRangeKeys is the threshold (as number of range keys)
-	// beyond which we'll clear range data using a single RANGEKEYDEL across the
-	// span rather than clearing individual range keys.
-	ClearRangeThresholdRangeKeys = 8
 )
 
 // ClearRangeDataOptions specify which parts of a Replica are to be destroyed.
@@ -78,14 +73,14 @@ func ClearRangeData(
 		UnreplicatedByRangeID: opts.ClearUnreplicatedByRangeID,
 	})
 
-	pointKeyThreshold, rangeKeyThreshold := ClearRangeThresholdPointKeys, ClearRangeThresholdRangeKeys
+	pointKeyThreshold := ClearRangeThresholdPointKeys
 	if opts.MustUseClearRange {
-		pointKeyThreshold, rangeKeyThreshold = 1, 1
+		pointKeyThreshold = 1
 	}
 
 	for _, keySpan := range keySpans {
 		if err := storage.ClearRangeWithHeuristic(
-			ctx, reader, writer, keySpan.Key, keySpan.EndKey, pointKeyThreshold, rangeKeyThreshold,
+			ctx, reader, writer, keySpan.Key, keySpan.EndKey, pointKeyThreshold,
 		); err != nil {
 			return err
 		}

pkg/kv/kvserver/snapshot_apply_prepare.go

@@ -272,7 +272,6 @@ func clearSubsumedReplicaDiskData(
 			keySpans[i].EndKey,
 			totalKeySpans[i].EndKey,
 			kvstorage.ClearRangeThresholdPointKeys,
-			kvstorage.ClearRangeThresholdRangeKeys,
 		); err != nil {
 			subsumedReplSST.Close()
 			return nil, err

pkg/storage/engine.go

@@ -1589,24 +1589,22 @@ func WriteSyncNoop(eng Engine) error {
 // either write a Pebble range tombstone or clear individual keys. If it uses
 // a range tombstone, it will tighten the span to the first encountered key.
 //
-// pointKeyThreshold and rangeKeyThreshold specify the number of point/range
-// keys respectively where it will switch from clearing individual keys to
-// Pebble range tombstones (RANGEDEL or RANGEKEYDEL respectively). A threshold
-// of 0 disables checking for and clearing that key type.
+// The pointKeyThreshold parameter specifies the number of point keys where it
+// will switch from clearing individual keys using point tombstones to clearing
+// the entire range using Pebble range tombstones (RANGEDELs). The
+// pointKeyThreshold value must be at least 1. NB: An initial scan will be done
+// to determine the type of clear, so a large threshold will potentially involve
+// scanning a large number of keys.
 //
-// NB: An initial scan will be done to determine the type of clear, so a large
-// threshold will potentially involve scanning a large number of keys twice.
-//
-// TODO(erikgrinaker): Consider tightening the end of the range tombstone span
-// too, by doing a SeekLT when we reach the threshold. It's unclear whether it's
-// really worth it.
+// ClearRangeWithHeuristic will also check for the existence of range keys, and
+// if any exist, it will write a RANGEKEYDEL clearing all range keys in the span.
 func ClearRangeWithHeuristic(
-	ctx context.Context,
-	r Reader,
-	w Writer,
-	start, end roachpb.Key,
-	pointKeyThreshold, rangeKeyThreshold int,
+	ctx context.Context, r Reader, w Writer, start, end roachpb.Key, pointKeyThreshold int,
 ) error {
+	if pointKeyThreshold < 1 {
+		return errors.AssertionFailedf("pointKeyThreshold must be at least 1")
+	}
+
 	clearPointKeys := func(r Reader, w Writer, start, end roachpb.Key, threshold int) error {
 		iter, err := r.NewEngineIterator(ctx, IterOptions{
 			KeyTypes:   IterKeyTypePointsOnly,
@@ -1655,7 +1653,7 @@ func ClearRangeWithHeuristic(
 		return err
 	}
 
-	clearRangeKeys := func(r Reader, w Writer, start, end roachpb.Key, threshold int) error {
+	clearRangeKeys := func(r Reader, w Writer, start, end roachpb.Key) error {
 		iter, err := r.NewEngineIterator(ctx, IterOptions{
 			KeyTypes:   IterKeyTypeRangesOnly,
 			LowerBound: start,
@@ -1666,51 +1664,29 @@ func ClearRangeWithHeuristic(
 		}
 		defer iter.Close()
 
-		// Scan, and drop a RANGEKEYDEL if we reach the threshold.
-		var ok bool
-		var count int
-		var firstKey roachpb.Key
-		for ok, err = iter.SeekEngineKeyGE(EngineKey{Key: start}); ok; ok, err = iter.NextEngineKey() {
-			count += len(iter.EngineRangeKeys())
-			if len(firstKey) == 0 {
-				bounds, err := iter.EngineRangeBounds()
-				if err != nil {
-					return err
-				}
-				firstKey = bounds.Key.Clone()
-			}
-			if count >= threshold {
-				return w.ClearRawRange(firstKey, end, false /* pointKeys */, true /* rangeKeys */)
-			}
-		}
-		if err != nil || count == 0 {
+		ok, err := iter.SeekEngineKeyGE(EngineKey{Key: start})
+		if err != nil {
 			return err
 		}
-		// Clear individual range keys.
-		for ok, err = iter.SeekEngineKeyGE(EngineKey{Key: start}); ok; ok, err = iter.NextEngineKey() {
-			bounds, err := iter.EngineRangeBounds()
-			if err != nil {
-				return err
-			}
-			for _, v := range iter.EngineRangeKeys() {
-				if err := w.ClearEngineRangeKey(bounds.Key, bounds.EndKey, v.Version); err != nil {
-					return err
-				}
-			}
+		if !ok {
+			// No range keys in the span.
+			return nil
 		}
-		return err
-	}
-
-	if pointKeyThreshold > 0 {
-		if err := clearPointKeys(r, w, start, end, pointKeyThreshold); err != nil {
+		bounds, err := iter.EngineRangeBounds()
+		if err != nil {
 			return err
 		}
+		// TODO(erikgrinaker): Consider tightening the end of the range
+		// tombstone span too, by doing a SeekLT when we reach the threshold.
+		// It's unclear whether it's really worth it.
+		return w.ClearRawRange(bounds.Key, end, false /* pointKeys */, true /* rangeKeys */)
 	}
 
-	if rangeKeyThreshold > 0 {
-		if err := clearRangeKeys(r, w, start, end, rangeKeyThreshold); err != nil {
-			return err
-		}
+	if err := clearPointKeys(r, w, start, end, pointKeyThreshold); err != nil {
+		return err
+	}
+	if err := clearRangeKeys(r, w, start, end); err != nil {
+		return err
 	}
 
 	return nil

pkg/storage/engine_test.go

@@ -1813,49 +1813,25 @@ func TestEngineClearRange(t *testing.T) {
 
 		"ClearRangeWithHeuristic individual": {
 			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, math.MaxInt, math.MaxInt)
+				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, math.MaxInt)
 			},
 			clearsPointKeys: true,
 			clearsRangeKeys: true,
 			clearsIntents:   false,
 		},
 		"ClearRangeWithHeuristic ranged": {
 			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, 1, 1)
+				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, 1)
 			},
 			clearsPointKeys: true,
 			clearsRangeKeys: true,
 			clearsIntents:   false,
 		},
 		"ClearRangeWithHeuristic point keys individual": {
 			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, math.MaxInt, 0)
+				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, math.MaxInt)
 			},
 			clearsPointKeys: true,
-			clearsRangeKeys: false,
-			clearsIntents:   false,
-		},
-		"ClearRangeWithHeuristic point keys ranged": {
-			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, 1, 0)
-			},
-			clearsPointKeys: true,
-			clearsRangeKeys: false,
-			clearsIntents:   false,
-		},
-		"ClearRangeWithHeuristic range keys individual": {
-			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, 0, math.MaxInt)
-			},
-			clearsPointKeys: false,
-			clearsRangeKeys: true,
-			clearsIntents:   false,
-		},
-		"ClearRangeWithHeuristic range keys ranged": {
-			clearRange: func(rw ReadWriter, start, end roachpb.Key) error {
-				return ClearRangeWithHeuristic(ctx, rw, rw, start, end, 0, 1)
-			},
-			clearsPointKeys: false,
 			clearsRangeKeys: true,
 			clearsIntents:   false,
 		},