Merge cockroachdb#98979

98979: sql: backward-compatibility for SHOW RANGES / crdb_internal.ranges r=cucaroach,fqazi a=knz

Requested by `@mwang1026`  and `@nvanbenschoten` .

Fixes cockroachdb#97861.
Fixes cockroachdb#99073.
Needed for cockroachdb#98820.
Epic: CRDB-24928

See the individual commits for details; exceprt from the 2nd commit:

TLDR: the pre-v23.1 behavior of SHOW RANGES and
`crdb_internal.ranges{_no_leases}` is made available conditional on a
new cluster setting `sql.show_ranges_deprecated_behavior.enabled`.

It is set to true by default in v23.1, however any use of the feature
will prompt a SQL notice with the following message:

```
NOTICE: attention! the pre-23.1 behavior of SHOW RANGES and crdb_internal.ranges{,_no_leases} is deprecated!
HINT: Consider enabling the new functionality by setting 'sql.show_ranges_deprecated_behavior.enabled' to 'false'.
The new SHOW RANGES statement has more options. Refer to the online
documentation or execute 'SHOW RANGES ??' for details.
```

The deprecated behavior is also disabled in the test suite and by default in
`cockroach demo`, i.e. the tests continue to exercise the new
behavior.

Release note (backward-incompatible change): The pre-v23.1 output
produced by `SHOW RANGES`, `crdb_internal.ranges`,
`crdb_internal.ranges_no_leases` is deprecated. The new interface and
functionality for `SHWO RANGES` can be enabled by changing the cluster
setting `sql.show_ranges_deprecated_behavior.enabled` to `false`. It
will become default in v23.2.


Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -251,6 +251,7 @@ sql.notices.enabled	boolean	true	enable notices in the server/client protocol be
 sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled	boolean	false	if enabled, uniqueness checks may be planned for mutations of UUID columns updated with gen_random_uuid(); otherwise, uniqueness is assumed due to near-zero collision probability
 sql.pgwire.multiple_active_portals.enabled	boolean	false	if true, portals with read-only SELECT query without sub/post queries can be executed in interleaving manner, but with local execution plan
 sql.schema.telemetry.recurrence	string	@weekly	cron-tab recurrence for SQL schema telemetry job
+sql.show_ranges_deprecated_behavior.enabled	boolean	true	if set, SHOW RANGES and crdb_internal.ranges{_no_leases} behave with deprecated pre-v23.1 semantics. NB: the new SHOW RANGES interface has richer WITH options than pre-v23.1 SHOW RANGES.
 sql.spatial.experimental_box2d_comparison_operators.enabled	boolean	false	enables the use of certain experimental box2d comparison operators
 sql.stats.automatic_collection.enabled	boolean	true	automatic statistics collection mode
 sql.stats.automatic_collection.fraction_stale_rows	float	0.2	target fraction of stale rows per table that will trigger a statistics refresh

docs/generated/settings/settings.html

@@ -202,6 +202,7 @@
 <tr><td><div id="setting-sql-optimizer-uniqueness-checks-for-gen-random-uuid-enabled" class="anchored"><code>sql.optimizer.uniqueness_checks_for_gen_random_uuid.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if enabled, uniqueness checks may be planned for mutations of UUID columns updated with gen_random_uuid(); otherwise, uniqueness is assumed due to near-zero collision probability</td></tr>
 <tr><td><div id="setting-sql-pgwire-multiple-active-portals-enabled" class="anchored"><code>sql.pgwire.multiple_active_portals.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if true, portals with read-only SELECT query without sub/post queries can be executed in interleaving manner, but with local execution plan</td></tr>
 <tr><td><div id="setting-sql-schema-telemetry-recurrence" class="anchored"><code>sql.schema.telemetry.recurrence</code></div></td><td>string</td><td><code>@weekly</code></td><td>cron-tab recurrence for SQL schema telemetry job</td></tr>
+<tr><td><div id="setting-sql-show-ranges-deprecated-behavior-enabled" class="anchored"><code>sql.show_ranges_deprecated_behavior.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, SHOW RANGES and crdb_internal.ranges{_no_leases} behave with deprecated pre-v23.1 semantics. NB: the new SHOW RANGES interface has richer WITH options than pre-v23.1 SHOW RANGES.</td></tr>
 <tr><td><div id="setting-sql-spatial-experimental-box2d-comparison-operators-enabled" class="anchored"><code>sql.spatial.experimental_box2d_comparison_operators.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables the use of certain experimental box2d comparison operators</td></tr>
 <tr><td><div id="setting-sql-stats-automatic-collection-enabled" class="anchored"><code>sql.stats.automatic_collection.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>automatic statistics collection mode</td></tr>
 <tr><td><div id="setting-sql-stats-automatic-collection-fraction-stale-rows" class="anchored"><code>sql.stats.automatic_collection.fraction_stale_rows</code></div></td><td>float</td><td><code>0.2</code></td><td>target fraction of stale rows per table that will trigger a statistics refresh</td></tr>

pkg/BUILD.bazel

@@ -1641,6 +1641,7 @@ GO_TARGETS = [
     "//pkg/sql/decodeusername:decodeusername",
     "//pkg/sql/decodeusername:decodeusername_test",
     "//pkg/sql/delegate:delegate",
+    "//pkg/sql/deprecatedshowranges:deprecatedshowranges",
     "//pkg/sql/descmetadata:descmetadata",
     "//pkg/sql/descmetadata:descmetadata_test",
     "//pkg/sql/distsql:distsql",
@@ -2924,6 +2925,7 @@ GET_X_DATA_TARGETS = [
     "//pkg/sql/covering:get_x_data",
     "//pkg/sql/decodeusername:get_x_data",
     "//pkg/sql/delegate:get_x_data",
+    "//pkg/sql/deprecatedshowranges:get_x_data",
     "//pkg/sql/descmetadata:get_x_data",
     "//pkg/sql/distsql:get_x_data",
     "//pkg/sql/doctor:get_x_data",

pkg/acceptance/cluster/dockercluster.go

@@ -493,6 +493,9 @@ func (l *DockerCluster) startNode(ctx context.Context, node *testNode) {
 		"COCKROACH_SCAN_MAX_IDLE_TIME=200ms",
 		"COCKROACH_SKIP_UPDATE_CHECK=1",
 		"COCKROACH_CRASH_REPORTS=",
+		// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+		// Remove in v23.2.
+		"COCKROACH_FORCE_DEPRECATED_SHOW_RANGE_BEHAVIOR=false",
 	}
 	l.createRoach(ctx, node, l.vols, env, cmd...)
 	maybePanic(node.Start(ctx))

pkg/cmd/roachtest/cluster.go

@@ -1837,6 +1837,14 @@ func (c *clusterImpl) StartE(
 		settings.Env = append(settings.Env, "COCKROACH_CRASH_ON_SPAN_USE_AFTER_FINISH=true")
 	}
 
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	if !envExists(settings.Env, "COCKROACH_FORCE_DEPRECATED_SHOW_RANGE_BEHAVIOR") {
+		// This makes all roachtest use the new SHOW RANGES behavior,
+		// regardless of cluster settings.
+		settings.Env = append(settings.Env, "COCKROACH_FORCE_DEPRECATED_SHOW_RANGE_BEHAVIOR=false")
+	}
+
 	clusterSettingsOpts := []install.ClusterSettingOption{
 		install.TagOption(settings.Tag),
 		install.PGUrlCertsDirOption(settings.PGUrlCertsDir),

pkg/server/BUILD.bazel

@@ -208,6 +208,7 @@ go_library(
         "//pkg/sql/consistencychecker",
         "//pkg/sql/contention",
         "//pkg/sql/contentionpb",
+        "//pkg/sql/deprecatedshowranges",
         "//pkg/sql/distsql",
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",

pkg/server/admin.go

@@ -48,6 +48,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/deprecatedshowranges"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
@@ -1054,11 +1055,50 @@ func (s *adminServer) tableDetailsHelper(
 	}
 
 	// MVCC Garbage result.
-	row, cols, err = s.internalExecutor.QueryRowExWithCols(
-		ctx, "admin-show-mvcc-garbage-info", nil,
-		sessiondata.InternalExecutorOverride{User: userName},
-		fmt.Sprintf(
+
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	if deprecatedshowranges.ShowRangesDeprecatedBehaviorSetting.Get(&s.st.SV) {
+		// NOTE: this query is deprecated. See the "else" branch below for
+		// the current code.
+		//
+		// This query is unable to handle table names with a schema qualification.
+		// If a user complains about this, tell them to opt out of the deprecated
+		// SHOW RANGES behavior, which will enable the proper semantics below.
+		tbName := strings.TrimPrefix(req.Table, "public.")
+		row, cols, err = s.internalExecutor.QueryRowExWithCols(
+			ctx, "admin-show-mvcc-garbage-info", nil,
+			sessiondata.InternalExecutorOverride{User: userName},
 			`WITH
+			range_stats AS (
+				SELECT crdb_internal.range_stats(start_key) AS d
+				FROM crdb_internal.ranges_no_leases WHERE database_name = $1 AND table_name = $2
+			),
+			aggregated AS (
+				SELECT
+					sum((d->>'live_bytes')::INT8) AS live,
+					sum(
+						(d->>'key_bytes')::INT8 + 
+						(d->>'val_bytes')::INT8 + 
+						COALESCE((d->>'range_key_bytes')::INT8, 0) +
+						COALESCE((d->>'range_val_bytes')::INT8, 0) +
+						(d->>'sys_bytes')::INT8) AS total
+				FROM
+					range_stats
+			)
+			SELECT
+				COALESCE(total, 0)::INT8 as total_bytes,
+				COALESCE(live, 0)::INT8 as live_bytes,
+				COALESCE(live / NULLIF(total,0), 0)::FLOAT8 as live_percentage
+			FROM aggregated`,
+			req.Database, tbName,
+		)
+	} else {
+		row, cols, err = s.internalExecutor.QueryRowExWithCols(
+			ctx, "admin-show-mvcc-garbage-info", nil,
+			sessiondata.InternalExecutorOverride{User: userName},
+			fmt.Sprintf(
+				`WITH
 			range_stats AS (
 				SELECT crdb_internal.range_stats(raw_start_key) AS d
 				FROM [SHOW RANGES FROM TABLE %s WITH KEYS]
@@ -1080,8 +1120,9 @@ func (s *adminServer) tableDetailsHelper(
 				COALESCE(live, 0)::INT8 as live_bytes,
 				COALESCE(live / NULLIF(total,0), 0)::FLOAT8 as live_percentage
 			FROM aggregated`,
-			escQualTable),
-	)
+				escQualTable),
+		)
+	}
 	if err != nil {
 		return nil, err
 	}

pkg/server/settingswatcher/BUILD.bazel

@@ -65,6 +65,7 @@ go_test(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/deprecatedshowranges",
         "//pkg/sql/sem/tree",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/deprecatedshowranges"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -135,6 +136,14 @@ func TestSettingWatcherOnTenant(t *testing.T) {
 	s0 := tc.Server(0)
 	tenantSettings := cluster.MakeTestingClusterSettings()
 	tenantSettings.SV.SetNonSystemTenant()
+
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	deprecatedshowranges.ShowRangesDeprecatedBehaviorSetting.Override(
+		ctx, &tenantSettings.SV,
+		// In unit tests, we exercise the new behavior.
+		false)
+
 	storage := &fakeStorage{}
 	sw := settingswatcher.New(s0.Clock(), fakeCodec, tenantSettings,
 		s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory,
@@ -373,6 +382,14 @@ func TestOverflowRestart(t *testing.T) {
 	defer s.Stopper().Stop(ctx)
 
 	sideSettings := cluster.MakeTestingClusterSettings()
+
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	deprecatedshowranges.ShowRangesDeprecatedBehaviorSetting.Override(
+		ctx, &sideSettings.SV,
+		// In unit tests, we exercise the new behavior.
+		false)
+
 	w := settingswatcher.New(
 		s.Clock(),
 		s.ExecutorConfig().(sql.ExecutorConfig).Codec,

pkg/server/testserver.go

@@ -45,6 +45,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
+	"github.com/cockroachdb/cockroach/pkg/sql/deprecatedshowranges"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -137,6 +138,14 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	if params.Settings == nil {
 		st = cluster.MakeClusterSettings()
 	}
+
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	deprecatedshowranges.ShowRangesDeprecatedBehaviorSetting.Override(
+		context.TODO(), &st.SV,
+		// In unit tests, we exercise the new behavior.
+		false)
+
 	st.ExternalIODir = params.ExternalIODir
 	tr := params.Tracer
 	if params.Tracer == nil {
@@ -1001,6 +1010,13 @@ func (ts *TestServer) StartTenant(
 		st = cluster.MakeTestingClusterSettings()
 	}
 
+	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
+	// Remove in v23.2.
+	deprecatedshowranges.ShowRangesDeprecatedBehaviorSetting.Override(
+		context.TODO(), &st.SV,
+		// In unit tests, we exercise the new behavior.
+		false)
+
 	st.ExternalIODir = params.ExternalIODir
 	sqlCfg := makeTestSQLConfig(st, params.TenantID)
 	sqlCfg.TenantKVAddrs = []string{ts.ServingRPCAddr()}

pkg/spanconfig/spanconfigstore/span_store.go

@@ -34,10 +34,10 @@ var tenantCoalesceAdjacentSetting = settings.RegisterBoolSetting(
 	true,
 )
 
-// storageCoalesceAdjacentSetting is a hidden cluster setting that
+// StorageCoalesceAdjacentSetting is a hidden cluster setting that
 // controls whether we coalesce adjacent ranges outside of the
 // secondary tenant keyspaces if they have the same span config.
-var storageCoalesceAdjacentSetting = settings.RegisterBoolSetting(
+var StorageCoalesceAdjacentSetting = settings.RegisterBoolSetting(
 	settings.SystemOnly,
 	"spanconfig.storage_coalesce_adjacent.enabled",
 	`collapse adjacent ranges with the same span configs for the ranges specific to the system tenant`,
@@ -152,7 +152,7 @@ func (s *spanConfigStore) computeSplitKey(start, end roachpb.RKey) (roachpb.RKey
 			// ranges.
 			systemTableUpperBound := keys.SystemSQLCodec.TablePrefix(keys.MaxReservedDescID + 1)
 			if roachpb.Key(rem).Compare(systemTableUpperBound) < 0 ||
-				!storageCoalesceAdjacentSetting.Get(&s.settings.SV) {
+				!StorageCoalesceAdjacentSetting.Get(&s.settings.SV) {
 				return roachpb.RKey(match.span.Key), nil
 			}
 		} else {

pkg/sql/BUILD.bazel

@@ -56,6 +56,7 @@ go_library(
         "copy_from.go",
         "copy_to.go",
         "crdb_internal.go",
+        "crdb_internal_ranges_deprecated.go",
         "create_database.go",
         "create_extension.go",
         "create_external_connection.go",
@@ -105,6 +106,7 @@ go_library(
         "drop_tenant.go",
         "drop_type.go",
         "drop_view.go",
+        "error_hints.go",
         "error_if_rows.go",
         "event_log.go",
         "exec_factory_util.go",
@@ -388,6 +390,7 @@ go_library(
         "//pkg/sql/covering",
         "//pkg/sql/decodeusername",
         "//pkg/sql/delegate",
+        "//pkg/sql/deprecatedshowranges",
         "//pkg/sql/descmetadata",
         "//pkg/sql/distsql",
         "//pkg/sql/enum",

pkg/sql/conn_executor.go

@@ -681,7 +681,7 @@ func (s *Server) getScrubbedStmtStats(
 	var scrubbedStats []appstatspb.CollectedStatementStatistics
 	stmtStatsVisitor := func(_ context.Context, stat *appstatspb.CollectedStatementStatistics) error {
 		// Scrub the statement itself.
-		scrubbedQueryStr, ok := scrubStmtStatKey(s.cfg.VirtualSchemas, stat.Key.Query)
+		scrubbedQueryStr, ok := scrubStmtStatKey(s.cfg.VirtualSchemas, stat.Key.Query, nil)
 
 		// We don't want to report this stats if scrubbing has failed. We also don't
 		// wish to abort here because we want to try our best to report all the
@@ -1110,7 +1110,7 @@ func (ex *connExecutor) closeWrapper(ctx context.Context, recovered interface{})
 			// Embed the statement in the error object for the telemetry
 			// report below. The statement gets anonymized.
 			vt := ex.planner.extendedEvalCtx.VirtualSchemas
-			panicErr = WithAnonymizedStatement(panicErr, ex.curStmtAST, vt)
+			panicErr = WithAnonymizedStatement(panicErr, ex.curStmtAST, vt, nil)
 		}
 
 		// Report the panic to telemetry in any case.
@@ -4217,7 +4217,7 @@ func init() {
 			return ""
 		}
 		// Anonymize the statement for reporting.
-		return anonymizeStmtAndConstants(stmt, nil /* VirtualTabler */)
+		return anonymizeStmtAndConstants(stmt, nil /* VirtualTabler */, nil /* ClientNoticeSender */)
 	})
 	logcrash.RegisterTagFn("gist", func(ctx context.Context) string {
 		return planGistFromCtx(ctx)

pkg/sql/conn_executor_exec.go

@@ -1246,7 +1246,7 @@ func (ex *connExecutor) dispatchToExecutionEngine(
 	err := ex.makeExecPlan(ctx, planner)
 	defer planner.curPlan.close(ctx)
 
-	// include gist in error reports
+	// Include gist in error reports.
 	ctx = withPlanGist(ctx, planner.instrumentation.planGist.String())
 	if planner.extendedEvalCtx.TxnImplicit {
 		planner.curPlan.flags.Set(planFlagImplicitTxn)
@@ -1278,6 +1278,7 @@ func (ex *connExecutor) dispatchToExecutionEngine(
 
 	// Finally, process the planning error from above.
 	if err != nil {
+		err = addPlanningErrorHints(ctx, err, &stmt, ex.server.cfg.Settings, planner)
 		res.SetError(err)
 		return nil
 	}

pkg/sql/conn_executor_test.go

@@ -84,7 +84,7 @@ INSERT INTO sensitive(super, sensible) VALUES('that', 'nobody', 'must', 'see')
 	}
 
 	rUnsafe := errors.New("some error")
-	safeErr := sql.WithAnonymizedStatement(rUnsafe, stmt1.AST, vt)
+	safeErr := sql.WithAnonymizedStatement(rUnsafe, stmt1.AST, vt, nil /* ClientNoticeSender */)
 
 	const expMessage = "some error"
 	actMessage := safeErr.Error()

pkg/sql/crdb_internal.go

@@ -1502,7 +1502,7 @@ CREATE TABLE crdb_internal.node_statement_statistics (
 
 		statementVisitor := func(_ context.Context, stats *appstatspb.CollectedStatementStatistics) error {
 			anonymized := tree.DNull
-			anonStr, ok := scrubStmtStatKey(p.getVirtualTabler(), stats.Key.Query)
+			anonStr, ok := scrubStmtStatKey(p.getVirtualTabler(), stats.Key.Query, p)
 			if ok {
 				anonymized = tree.NewDString(anonStr)
 			}