Merge cockroachdb#99186 cockroachdb#99246

99186: builtins: add hint for hide_sql_constants and redactable_sql_constants r=msirek a=michae2

Add a hint about using dollar-quotes to the info message for builtin functions `crdb_internal.hide_sql_constants` and
`crdb_internal.redactable_sql_constants`.

Fixes: cockroachdb#99132

Epic: None

Release note: None

99246: multitenant: add tenant end key split to MetadataSchema r=knz a=ecwall

Fixes cockroachdb#97985

Previously cockroachdb#95100 created tenant end key split points asynchronously which
allowed for a brief period after tenant creation where the tenant's keyspace
had an end key of /Max instead of the next tenant's start key.

This change creates that split point synchronously on tenant creation to avoid
race conditions.

Release note: None


Co-authored-by: Michael Erickson <[email protected]>
Co-authored-by: Evan Wall <[email protected]>

pkg/keys/sql.go

@@ -121,7 +121,7 @@ func (e sqlEncoder) TenantPrefix() roachpb.Key {
 	return *e.buf
 }
 
-// TenantPrefix returns the key prefix used for the tenants's data.
+// TenantSpan returns a span representing the tenant's keyspace.
 func (e sqlEncoder) TenantSpan() roachpb.Span {
 	key := *e.buf
 	return roachpb.Span{Key: key, EndKey: key.PrefixEnd()}

pkg/multitenant/mtinfopb/info.go

@@ -96,7 +96,7 @@ var TenantDataStateValues = map[string]TenantDataState{
 
 // TenantInfo captures both a ProtoInfo and the SQLInfo columns that
 // go alongside it, sufficient to represent an entire row in
-// system.tenans.
+// system.tenants.
 type TenantInfo struct {
 	ProtoInfo
 	SQLInfo

pkg/server/BUILD.bazel

@@ -260,7 +260,6 @@ go_library(
         "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/storage/fs",
-        "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",
         "//pkg/ts",

pkg/server/testserver.go

@@ -50,7 +50,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/storage"
-	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/ts"
@@ -62,7 +61,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	addrutil "github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
-	"github.com/cockroachdb/cockroach/pkg/util/rangedesc"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
@@ -820,31 +818,6 @@ func (t *TestTenant) SettingsWatcher() interface{} {
 	return t.SQLServer.settingsWatcher
 }
 
-// WaitForTenantEndKeySplit is part of the TestTenantInterface.
-func (t *TestTenant) WaitForTenantEndKeySplit(ctx context.Context) error {
-	// Wait until the tenant end key split happens.
-	return testutils.SucceedsWithinError(func() error {
-		factory := t.RangeDescIteratorFactory().(rangedesc.IteratorFactory)
-
-		iterator, err := factory.NewIterator(ctx, t.Codec().TenantSpan())
-		if err != nil {
-			return err
-		}
-		if !iterator.Valid() {
-			return errors.New("range iterator has no ranges")
-		}
-
-		for iterator.Valid() {
-			rangeDesc := iterator.CurRangeDescriptor()
-			if rangeDesc.EndKey.Compare(roachpb.RKeyMax) == 0 {
-				return errors.Newf("range ID %d end key not split", rangeDesc.RangeID)
-			}
-			iterator.Next()
-		}
-		return nil
-	}, 10*time.Second)
-}
-
 // StartSharedProcessTenant is part of TestServerInterface.
 func (ts *TestServer) StartSharedProcessTenant(
 	ctx context.Context, args base.TestSharedProcessTenantArgs,
@@ -1668,12 +1641,6 @@ func (ts *TestServer) Tracer() *tracing.Tracer {
 	return ts.node.storeCfg.AmbientCtx.Tracer
 }
 
-// WaitForTenantEndKeySplit is part of the TestTenantInterface.
-func (ts *TestServer) WaitForTenantEndKeySplit(context.Context) error {
-	// Does not apply to system tenant.
-	return nil
-}
-
 // ForceTableGC is part of TestServerInterface.
 func (ts *TestServer) ForceTableGC(
 	ctx context.Context, database, table string, timestamp hlc.Timestamp,

pkg/sql/catalog/bootstrap/metadata.go

@@ -232,14 +232,16 @@ func (ms MetadataSchema) GetInitialValues() ([]roachpb.KeyValue, []roachpb.RKey)
 	// boundaries. In fact, if we tried to split at table boundaries, those
 	// splits would quickly be merged away. The only enforced split points are
 	// between secondary tenants (e.g. between /tenant/<id> and /tenant/<id+1>).
-	// So we drop all descriptor split points and replace it with a single split
-	// point at the beginning of this tenant's keyspace.
+	// So we drop all descriptor split points and replace them with split points
+	// at the beginning and end of this tenant's keyspace.
 	if ms.codec.ForSystemTenant() {
 		for _, id := range ms.otherSplitIDs {
 			splits = append(splits, roachpb.RKey(ms.codec.TablePrefix(id)))
 		}
 	} else {
-		splits = []roachpb.RKey{roachpb.RKey(ms.codec.TenantPrefix())}
+		tenantStartKey := roachpb.RKey(ms.codec.TenantPrefix())
+		tenantEndKey := tenantStartKey.PrefixEnd()
+		splits = []roachpb.RKey{tenantStartKey, tenantEndKey}
 	}
 
 	// Other key/value generation that doesn't fit into databases and
@@ -273,11 +275,16 @@ func InitialValuesToString(ms MetadataSchema) string {
 	for _, kv := range kvs {
 		records = append(records, record{k: kv.Key, v: kv.Value.TagAndDataBytes()})
 	}
+	p := ms.codec.TenantPrefix()
+	pNext := p.PrefixEnd()
 	for _, s := range splits {
+		// Filter out the tenant end key because it does not have the same prefix.
+		if bytes.HasPrefix(s, pNext) {
+			continue
+		}
 		records = append(records, record{k: s})
 	}
 	// Strip the tenant prefix if there is one.
-	p := []byte(ms.codec.TenantPrefix())
 	for i, r := range records {
 		if !bytes.Equal(p, r.k[:len(p)]) {
 			panic("unexpected prefix")
@@ -340,6 +347,10 @@ func InitialValuesFromString(
 			kvs = append(kvs, kv)
 		}
 	}
+	// Add back the filtered out tenant end key.
+	if !codec.ForSystemTenant() {
+		splits = append(splits, roachpb.RKey(p.PrefixEnd()))
+	}
 	return kvs, splits, nil
 }
 

pkg/sql/logictest/logic.go

@@ -1466,9 +1466,6 @@ func (t *logicTest) newCluster(
 			if err != nil {
 				t.rootT.Fatalf("%+v", err)
 			}
-			if err := tenant.WaitForTenantEndKeySplit(context.Background()); err != nil {
-				t.rootT.Fatalf("%+v", err)
-			}
 			t.tenantAddrs[i] = tenant.SQLAddr()
 		}
 

pkg/sql/multitenant_admin_function_test.go

@@ -258,7 +258,6 @@ func (tc testCase) runTest(
 		testServer.Stopper(),
 	)
 
-	var secondaryTenants []serverutils.TestTenantInterface
 	createSecondaryDB := func(
 		tenantID roachpb.TenantID,
 		clusterSettings ...*settings.BoolSetting,
@@ -270,13 +269,12 @@ func (tc testCase) runTest(
 				clusterSetting.Override(ctx, &testingClusterSettings.SV, true)
 			}
 		}
-		tenant, db := serverutils.StartTenant(
+		_, db := serverutils.StartTenant(
 			t, testServer, base.TestTenantArgs{
 				Settings: testingClusterSettings,
 				TenantID: tenantID,
 			},
 		)
-		secondaryTenants = append(secondaryTenants, tenant)
 		return db
 	}
 
@@ -339,12 +337,6 @@ func (tc testCase) runTest(
 		fn()
 	}
 
-	// Wait for splits after starting all tenants to make test start up faster.
-	for _, tenant := range secondaryTenants {
-		err := tenant.WaitForTenantEndKeySplit(ctx)
-		require.NoError(t, err)
-	}
-
 	execQueries(testCluster, systemDB, "system", tc.system)
 	if tc.secondary.isSet() {
 		execQueries(testCluster, secondaryDB, "secondary", tc.secondary)

pkg/sql/sem/builtins/builtins.go

@@ -7703,7 +7703,7 @@ expires until the statement bundle is collected`,
 			},
 			types.String,
 			"Removes constants from a SQL statement. String provided must contain at most "+
-				"1 statement.",
+				"1 statement. (Hint: one way to easily quote arbitrary SQL is to use dollar-quotes.)",
 			volatility.Immutable,
 		),
 		tree.Overload{
@@ -7740,7 +7740,8 @@ expires until the statement bundle is collected`,
 				return result, nil
 			},
 			Info: "Hide constants for each element in an array of SQL statements. " +
-				"Note that maximum 1 statement is permitted per string element.",
+				"Note that maximum 1 statement is permitted per string element. (Hint: one way to easily " +
+				"quote arbitrary SQL is to use dollar-quotes.)",
 			Volatility: volatility.Immutable,
 		},
 	),
@@ -7779,7 +7780,8 @@ expires until the statement bundle is collected`,
 			},
 			types.String,
 			"Surrounds constants in SQL statement with redaction markers. String provided must "+
-				"contain at most 1 statement.",
+				"contain at most 1 statement. (Hint: one way to easily quote arbitrary SQL is to use "+
+				"dollar-quotes.)",
 			volatility.Immutable,
 		),
 		tree.Overload{
@@ -7816,7 +7818,8 @@ expires until the statement bundle is collected`,
 				return result, nil
 			},
 			Info: "Surrounds constants with redaction markers for each element in an array of SQL " +
-				"statements. Note that maximum 1 statement is permitted per string element.",
+				"statements. Note that maximum 1 statement is permitted per string element. (Hint: one " +
+				"way to easily quote arbitrary SQL is to use dollar-quotes.)",
 			Volatility: volatility.Immutable,
 		},
 	),

pkg/sql/tests/testdata/initial_keys

@@ -275,8 +275,9 @@ initial-keys tenant=5
  /Tenant/5/NamespaceTable/30/1/1/29/"web_sessions"/4/1
  /Tenant/5/NamespaceTable/30/1/1/29/"zones"/4/1
  /Tenant/5/Table/48/1/0/0
-1 splits:
+2 splits:
  /Tenant/5
+ /Tenant/6
 
 initial-keys tenant=999
 ----
@@ -377,5 +378,6 @@ initial-keys tenant=999
  /Tenant/999/NamespaceTable/30/1/1/29/"web_sessions"/4/1
  /Tenant/999/NamespaceTable/30/1/1/29/"zones"/4/1
  /Tenant/999/Table/48/1/0/0
-1 splits:
+2 splits:
  /Tenant/999
+ /Tenant/1000

pkg/testutils/serverutils/test_tenant_shim.go

@@ -185,14 +185,6 @@ type TestTenantInterface interface {
 	// Tracer returns a reference to the tenant's Tracer.
 	Tracer() *tracing.Tracer
 
-	// WaitForTenantEndKeySplit blocks until the tenant's initial range is split
-	// at the end key. For example, this will wait until tenant 10 has a split at
-	// /Tenant/11.
-	//
-	// Tests that use crdb_internal.ranges, crdb_internal.ranges_no_leases, or
-	// SHOW RANGES from a secondary tenant should call this to avoid races.
-	WaitForTenantEndKeySplit(ctx context.Context) error
-
 	// MigrationServer returns the tenant's migration server, which is used in
 	// upgrade testing.
 	MigrationServer() interface{}