Skip to content

Commit

Permalink
sql: backward-compat for SHOW RANGES / `crdb_internal.ranges{,_no_l…
Browse files Browse the repository at this point in the history
…eases}`

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 in
`cockroach demo`, i.e. the tests continue to exercise the new
behavior.

cf. doc for new package `deprecatedshowranges`:

```
// Package deprecatedshowranges exists to smoothen the transition
// between the pre-v23.1 semantics of SHOW RANGES and
// crdb_internal.ranges{_no_leases}, and the new semantics introduced
// in v23.1.
//
// The pre-v23.1 semantics are deprecated as of v23.1. At the end of
// the deprecation cycle (hopefully for v23.2) we expect to delete
// this package entirely and all the other code in the SQL layer that
// hangs off the EnableDeprecatedBehavior() conditional defined below.
//
// The mechanism to control the behavior is as follows:
//
//   - In any case, an operator can override the behavior using an env
//     var, "COCKROACH_FORCE_DEPRECATED_SHOW_RANGE_BEHAVIOR".
//     If set (to a boolean), the value of the env var is used in
//     all cases.
//     We use this env var to force the _new_ behavior through out
//     test suite, regardless of the other conditions. This allows
//     us to avoid maintaining two sets of outputs in tests.
//
//   - If the env var is not set, the cluster setting
//     `sql.show_ranges_deprecated_behavior.enabled` is used. It
//     defaults to true in v23.1 (the "smoothening" part).
//
//   - If the deprecated behavior is chosen by any of the above
//     mechanisms, and _the range coalescing cluster setting_ is set
//     to true, a loud warning will be reported in various places.
//     This will nudge users who wish to opt into range coalescing
//     to adapt their use of the range inspection accordingly.
```

Release note: None
  • Loading branch information
knz committed Mar 22, 2023
1 parent 16bdc14 commit c575203
Show file tree
Hide file tree
Showing 25 changed files with 707 additions and 42 deletions.
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,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",
Expand Down Expand Up @@ -2922,6 +2923,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",
Expand Down
3 changes: 3 additions & 0 deletions pkg/acceptance/cluster/dockercluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
8 changes: 8 additions & 0 deletions pkg/cmd/roachtest/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions pkg/server/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 9 additions & 0 deletions pkg/server/testserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -139,6 +140,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 {
Expand Down
6 changes: 3 additions & 3 deletions pkg/spanconfig/spanconfigstore/span_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -389,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",
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ func (ex *connExecutor) dispatchToExecutionEngine(

// Finally, process the planning error from above.
if err != nil {
err = addPlanningErrorHints(err, &stmt)
err = addPlanningErrorHints(ctx, err, &stmt, ex.server.cfg.Settings, planner)
res.SetError(err)
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/conn_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/crdb_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Loading

0 comments on commit c575203

Please sign in to comment.