Merge #87265 #87957 #87987 #88058

87265: descs: unify by-name and by-ID lookups r=postamar a=postamar

This commit unifies the descriptor lookup logic in the descs.Collection
object without changing its behavior. By-name lookups now first
retrieve a descriptor ID and then perform a by-ID lookup. The by-ID
lookup logic is uniquely defined and is responsible for all necessary
validation, hydration and filtering.

This commit also fixes a longstanding hydration bug, in which
uncommitted tables were not properly re-hydrated following a schema
change of a column type in the same transaction.

This commit also changes the return type of GetMutableSchema* methods
from a catalog.SchemaDescriptor to a *schemadesc.Mutable. Previously
these methods would return mutable descriptors on a best-effort basis,
non-physical schema descriptors like temporary or virtual schemas
would remain immutable. Now, an error is returned instead in those
cases.

Release justification: no change to functionality
Release note: None


87957: opt: normalize JSON subscripts `[...]` to fetch value operators `->` r=faizaanmadhani a=faizaanmadhani

Previously, jsonb subscripts were not normalized to fetch value operators. This didn't allow queries with filters like `json_col['a'] = '1'` to scan inverted indexes, resulting in less efficient query plans. With this rule, these types of queries can be index accelerated.

Resolves: #83441

Release note (performance improvement): The optimizer will now plan inverted index scans for queries with JSON subscripting filters, like `json_col['field'] = '"value"`.

87987: storage: handle MVCC range keys in all `NewMVCCIterator` callers r=tbg a=erikgrinaker

**storage: handle inline values in `MVCCIsSpanEmpty`**

`MVCCIsSpanEmpty` uses an `MVCCIncrementalIterator` to handle time
bounds. However, this will error if it encounters any inline values.
This patch instead uses a regular MVCC iterator when time bounds are not
given (the typical case), which correctly handles inline values.

Release note: None

**batcheval: use `MVCCIsSpanEmpty` in `EndTxn`**

In addition to code deduplication, this also respects MVCC range
tombstones.

Release note: None
  
**kvserver: respect MVCC range keys in `optimizePuts`**

If many point writes are submitted, `optimizePuts()` looks for virgin
keyspace and switches to blind writes to amortize seek costs. This did
not check for MVCC range keys, which could lead to faulty conflict
checks and stats updates.

Release note: None
  
**storage: note functions that ignore MVCC range keys**

Release note: None

Touches #87366.

88058: cdc: test negative timestamp cdc r=biradarganesh25 a=biradarganesh25

The negative timestamp value is calculated as the difference between a
predefined time (this is the time from which we expect changefeed to run)
and the current statement time. knobs is used to get the current statement
time because it will only be available once the change feed statement
starts executing.

Resolves: #82350

Release note: None

Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Faizaan Madhani <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Ganeshprasad Rajashekhar Biradar <[email protected]>

pkg/ccl/changefeedccl/changefeed_stmt.go

@@ -328,6 +328,11 @@ func createChangefeedJobRecord(
 	}
 	var initialHighWater hlc.Timestamp
 	evalTimestamp := func(s string) (hlc.Timestamp, error) {
+		if knobs, ok := p.ExecCfg().DistSQLSrv.TestingKnobs.Changefeed.(*TestingKnobs); ok {
+			if knobs != nil && knobs.OverrideCursor != nil {
+				s = knobs.OverrideCursor(&statementTime)
+			}
+		}
 		asOfClause := tree.AsOfClause{Expr: tree.NewStrVal(s)}
 		asOf, err := p.EvalAsOfTimestamp(ctx, asOfClause)
 		if err != nil {

pkg/ccl/changefeedccl/changefeed_test.go

@@ -666,6 +666,7 @@ func TestChangefeedCursor(t *testing.T) {
 		// 'after', throw a couple sleeps around them. We round timestamps to
 		// Microsecond granularity for Postgres compatibility, so make the
 		// sleeps 10x that.
+		beforeInsert := s.Server.Clock().Now()
 		sqlDB.Exec(t, `INSERT INTO foo VALUES (1, 'before')`)
 		time.Sleep(10 * time.Microsecond)
 
@@ -677,6 +678,32 @@ func TestChangefeedCursor(t *testing.T) {
 		time.Sleep(10 * time.Microsecond)
 		sqlDB.Exec(t, `INSERT INTO foo VALUES (2, 'after')`)
 
+		// The below function is currently used to test negative timestamp in cursor i.e of the form
+		// "-3us".
+		// Using this function we can calculate the difference with the time that was before
+		// the insert statement, which is set as the new cursor value inside createChangefeedJobRecord
+		calculateCursor := func(currentTime *hlc.Timestamp) string {
+			//  Should convert to microseconds as that is the maximum precision we support
+			diff := (beforeInsert.WallTime - currentTime.WallTime) / 1000
+			diffStr := strconv.FormatInt(diff, 10) + "us"
+			return diffStr
+		}
+
+		knobs := s.TestingKnobs.DistSQL.(*execinfra.TestingKnobs).Changefeed.(*TestingKnobs)
+		knobs.OverrideCursor = calculateCursor
+
+		// The "-3 days" is a placeholder here - it will be replaced with actual difference
+		// in createChangefeedJobRecord
+		fooInterval := feed(t, f, `CREATE CHANGEFEED FOR foo WITH cursor=$1`, "-3 days")
+		defer closeFeed(t, fooInterval)
+		assertPayloads(t, fooInterval, []string{
+			`foo: [1]->{"after": {"a": 1, "b": "before"}}`,
+			`foo: [2]->{"after": {"a": 2, "b": "after"}}`,
+		})
+
+		// We do not need to override for the remaining cases
+		knobs.OverrideCursor = nil
+
 		fooLogical := feed(t, f, `CREATE CHANGEFEED FOR foo WITH cursor=$1`, tsLogical)
 		defer closeFeed(t, fooLogical)
 		assertPayloads(t, fooLogical, []string{

pkg/ccl/changefeedccl/testing_knobs.go

@@ -15,6 +15,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 )
 
@@ -47,6 +48,12 @@ type TestingKnobs struct {
 	ShouldReplan func(ctx context.Context, oldPlan, newPlan *sql.PhysicalPlan) bool
 	// RaiseRetryableError is a knob used to possibly return an error.
 	RaiseRetryableError func() error
+
+	// This is currently used to test negative timestamp in cursor i.e of the form
+	// "-3us". Check TestChangefeedCursor for more info. This function needs to be in the
+	// knobs as current statement time will only be available once the create changefeed statement
+	// starts executing.
+	OverrideCursor func(currentTime *hlc.Timestamp) string
 }
 
 // ModuleTestingKnobs is part of the base.ModuleTestingKnobs interface.

pkg/kv/kvserver/batcheval/cmd_end_transaction.go

@@ -947,7 +947,10 @@ func splitTrigger(
 	// TODO(nvanbenschoten): this is a simple heuristic. If we had a cheap way to
 	// determine the relative sizes of the LHS and RHS, we could be more
 	// sophisticated here and always choose to scan the cheaper side.
-	emptyRHS, err := isGlobalKeyspaceEmpty(batch, &split.RightDesc)
+	emptyRHS, err := storage.MVCCIsSpanEmpty(ctx, batch, storage.MVCCIsSpanEmptyOptions{
+		StartKey: split.RightDesc.StartKey.AsRawKey(),
+		EndKey:   split.RightDesc.EndKey.AsRawKey(),
+	})
 	if err != nil {
 		return enginepb.MVCCStats{}, result.Result{}, errors.Wrapf(err,
 			"unable to determine whether right hand side of split is empty")
@@ -978,21 +981,6 @@ func splitTrigger(
 var splitScansRightForStatsFirst = util.ConstantWithMetamorphicTestBool(
 	"split-scans-right-for-stats-first", false)
 
-// isGlobalKeyspaceEmpty returns whether the global keyspace of the provided
-// range is entirely empty. The function returns false if the global keyspace
-// contains at least one key.
-func isGlobalKeyspaceEmpty(reader storage.Reader, d *roachpb.RangeDescriptor) (bool, error) {
-	span := d.KeySpan().AsRawSpanWithNoLocals()
-	iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{UpperBound: span.EndKey})
-	defer iter.Close()
-	iter.SeekGE(storage.MakeMVCCMetadataKey(span.Key))
-	ok, err := iter.Valid()
-	if err != nil {
-		return false, err
-	}
-	return !ok /* empty */, nil
-}
-
 // makeScanStatsFn constructs a splitStatsScanFn for the provided post-split
 // range descriptor which computes the range's statistics.
 func makeScanStatsFn(

pkg/kv/kvserver/batcheval/cmd_is_span_empty.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/errors"
 )
 
@@ -35,8 +34,6 @@ func IsSpanEmpty(
 	isEmpty, err := storage.MVCCIsSpanEmpty(ctx, reader, storage.MVCCIsSpanEmptyOptions{
 		StartKey: args.Key,
 		EndKey:   args.EndKey,
-		StartTS:  hlc.MinTimestamp, // beginning of time
-		EndTS:    hlc.MaxTimestamp, // end of time
 	})
 	if err != nil {
 		return result.Result{}, errors.Wrap(err, "IsSpanEmpty")

pkg/kv/kvserver/replica_evaluate.go

@@ -92,6 +92,7 @@ func optimizePuts(
 	// don't need to see intents for this purpose since intents also have
 	// provisional values that we will see.
 	iter := reader.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
+		KeyTypes: storage.IterKeyTypePointsAndRanges,
 		// We want to include maxKey in our scan. Since UpperBound is exclusive, we
 		// need to set it to the key after maxKey.
 		UpperBound: maxKey.Next(),

pkg/kv/kvserver/replica_test.go

@@ -1771,12 +1771,13 @@ func TestOptimizePuts(t *testing.T) {
 
 	testCases := []struct {
 		exKey    roachpb.Key
+		exEndKey roachpb.Key // MVCC range key
 		reqs     []roachpb.Request
 		expBlind []bool
 	}{
 		// No existing keys, single put.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0],
 			},
@@ -1786,7 +1787,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// No existing keys, nine puts.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8],
 			},
@@ -1796,7 +1797,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// No existing keys, ten puts.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
 			},
@@ -1806,7 +1807,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at "0", ten conditional puts.
 		{
-			roachpb.Key("0"),
+			roachpb.Key("0"), nil,
 			[]roachpb.Request{
 				&cpArgs[0], &cpArgs[1], &cpArgs[2], &cpArgs[3], &cpArgs[4], &cpArgs[5], &cpArgs[6], &cpArgs[7], &cpArgs[8], &cpArgs[9],
 			},
@@ -1816,7 +1817,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at "0", ten init puts.
 		{
-			roachpb.Key("0"),
+			roachpb.Key("0"), nil,
 			[]roachpb.Request{
 				&ipArgs[0], &ipArgs[1], &ipArgs[2], &ipArgs[3], &ipArgs[4], &ipArgs[5], &ipArgs[6], &ipArgs[7], &ipArgs[8], &ipArgs[9],
 			},
@@ -1826,7 +1827,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at 11, mixed put types.
 		{
-			roachpb.Key("11"),
+			roachpb.Key("11"), nil,
 			[]roachpb.Request{
 				&pArgs[0], &cpArgs[1], &pArgs[2], &cpArgs[3], &ipArgs[4], &ipArgs[5], &pArgs[6], &cpArgs[7], &pArgs[8], &ipArgs[9],
 			},
@@ -1836,7 +1837,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at 00, ten puts, expect nothing blind.
 		{
-			roachpb.Key("00"),
+			roachpb.Key("00"), nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
 			},
@@ -1846,7 +1847,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at 00, ten puts in reverse order, expect nothing blind.
 		{
-			roachpb.Key("00"),
+			roachpb.Key("00"), nil,
 			[]roachpb.Request{
 				&pArgs[9], &pArgs[8], &pArgs[7], &pArgs[6], &pArgs[5], &pArgs[4], &pArgs[3], &pArgs[2], &pArgs[1], &pArgs[0],
 			},
@@ -1856,7 +1857,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at 05, ten puts, expect first five puts are blind.
 		{
-			roachpb.Key("05"),
+			roachpb.Key("05"), nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
 			},
@@ -1866,7 +1867,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Existing key at 09, ten puts, expect first nine puts are blind.
 		{
-			roachpb.Key("09"),
+			roachpb.Key("09"), nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
 			},
@@ -1876,7 +1877,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// No existing key, ten puts + inc + ten cputs.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
 				incArgs, &cpArgs[0], &cpArgs[1], &cpArgs[2], &cpArgs[3], &cpArgs[4], &cpArgs[5], &cpArgs[6], &cpArgs[7], &cpArgs[8], &cpArgs[9],
@@ -1888,7 +1889,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Duplicate put at 11th key; should see ten puts.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9], &pArgs[9],
 			},
@@ -1898,7 +1899,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Duplicate cput at 11th key; should see ten puts.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9], &cpArgs[9],
 			},
@@ -1908,7 +1909,7 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Duplicate iput at 11th key; should see ten puts.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9], &ipArgs[9],
 			},
@@ -1918,22 +1919,53 @@ func TestOptimizePuts(t *testing.T) {
 		},
 		// Duplicate cput at 10th key; should see ten cputs.
 		{
-			nil,
+			nil, nil,
 			[]roachpb.Request{
 				&cpArgs[0], &cpArgs[1], &cpArgs[2], &cpArgs[3], &cpArgs[4], &cpArgs[5], &cpArgs[6], &cpArgs[7], &cpArgs[8], &cpArgs[9], &cpArgs[9],
 			},
 			[]bool{
 				true, true, true, true, true, true, true, true, true, true, false,
 			},
 		},
+		// Existing range key at 00-20, ten puts, expect no blind.
+		{
+			roachpb.Key("00"), roachpb.Key("20"),
+			[]roachpb.Request{
+				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
+			},
+			[]bool{
+				false, false, false, false, false, false, false, false, false, false,
+			},
+		},
+		// Existing range key at 05-08, ten puts, expect first five puts are blind.
+		{
+			roachpb.Key("05"), roachpb.Key("08"),
+			[]roachpb.Request{
+				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
+			},
+			[]bool{
+				true, true, true, true, true, false, false, false, false, false,
+			},
+		},
+		// Existing range key at 20-21, ten puts, expect all blind.
+		{
+			roachpb.Key("20"), roachpb.Key("21"),
+			[]roachpb.Request{
+				&pArgs[0], &pArgs[1], &pArgs[2], &pArgs[3], &pArgs[4], &pArgs[5], &pArgs[6], &pArgs[7], &pArgs[8], &pArgs[9],
+			},
+			[]bool{
+				true, true, true, true, true, true, true, true, true, true,
+			},
+		},
 	}
 
 	for i, c := range testCases {
-		if c.exKey != nil {
-			if err := storage.MVCCPut(context.Background(), tc.engine, nil, c.exKey,
-				hlc.Timestamp{}, hlc.ClockTimestamp{}, roachpb.MakeValueFromString("foo"), nil); err != nil {
-				t.Fatal(err)
-			}
+		if c.exEndKey != nil {
+			require.NoError(t, storage.MVCCDeleteRangeUsingTombstone(ctx, tc.engine, nil,
+				c.exKey, c.exEndKey, hlc.MinTimestamp, hlc.ClockTimestamp{}, nil, nil, false, 0, nil))
+		} else if c.exKey != nil {
+			require.NoError(t, storage.MVCCPut(ctx, tc.engine, nil, c.exKey,
+				hlc.Timestamp{}, hlc.ClockTimestamp{}, roachpb.MakeValueFromString("foo"), nil))
 		}
 		batch := roachpb.BatchRequest{}
 		for _, r := range c.reqs {
@@ -1976,10 +2008,11 @@ func TestOptimizePuts(t *testing.T) {
 		if !reflect.DeepEqual(blind, c.expBlind) {
 			t.Errorf("%d: expected %+v; got %+v", i, c.expBlind, blind)
 		}
-		if c.exKey != nil {
-			if err := tc.engine.ClearUnversioned(c.exKey); err != nil {
-				t.Fatal(err)
-			}
+		if c.exEndKey != nil {
+			require.NoError(t, tc.engine.ClearMVCCRangeKey(storage.MVCCRangeKey{
+				StartKey: c.exKey, EndKey: c.exEndKey, Timestamp: hlc.MinTimestamp}))
+		} else if c.exKey != nil {
+			require.NoError(t, tc.engine.ClearUnversioned(c.exKey))
 		}
 	}
 }

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go

@@ -503,10 +503,10 @@ func (s *SQLTranslator) findDescendantLeafIDs(
 // findDescendantLeafIDsForDescriptor finds all leaf object IDs below the given
 // descriptor ID in the zone configuration hierarchy. Based on the descriptor
 // type, these are:
-// - Database: IDs of all tables inside the database.
-// - Table: ID of the table itself.
-// - Schema/Type: Nothing, as schemas/types do not carry zone configurations and
-// are not part of the zone configuration hierarchy.
+//   - Database: IDs of all tables inside the database.
+//   - Table: ID of the table itself.
+//   - Other: Nothing, as these do not carry zone configurations and
+//     are not part of the zone configuration hierarchy.
 func (s *SQLTranslator) findDescendantLeafIDsForDescriptor(
 	ctx context.Context, id descpb.ID, txn *kv.Txn, descsCol *descs.Collection,
 ) (descpb.IDs, error) {
@@ -521,30 +521,29 @@ func (s *SQLTranslator) findDescendantLeafIDsForDescriptor(
 		return nil, nil // we're excluding this descriptor; nothing to do here
 	}
 
-	switch desc.DescriptorType() {
-	case catalog.Type, catalog.Schema:
-		// There is nothing to do for {Type, Schema} descriptors as they are not
-		// part of the zone configuration hierarchy.
-		return nil, nil
-	case catalog.Table:
+	var db catalog.DatabaseDescriptor
+	switch t := desc.(type) {
+	case catalog.TableDescriptor:
 		// Tables are leaf objects in the zone configuration hierarchy, so simply
 		// return the ID.
 		return descpb.IDs{id}, nil
-	case catalog.Database:
-	// Fallthrough.
+	case catalog.DatabaseDescriptor:
+		db = t
 	default:
-		return nil, errors.AssertionFailedf("unknown descriptor type: %s", desc.DescriptorType())
+		// There is nothing to do for non-table-or-database descriptors as they are
+		// not part of the zone configuration hierarchy.
+		return nil, nil
 	}
 
 	// There's nothing for us to do if the descriptor is offline or has been
 	// dropped.
-	if desc.Offline() || desc.Dropped() {
+	if db.Offline() || db.Dropped() {
 		return nil, nil
 	}
 
 	// Expand the database descriptor to all the tables inside it and return their
 	// IDs.
-	tables, err := descsCol.GetAllTableDescriptorsInDatabase(ctx, txn, desc.GetID())
+	tables, err := descsCol.GetAllTableDescriptorsInDatabase(ctx, txn, db)
 	if err != nil {
 		return nil, err
 	}