Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-23.1: sql: backward-compatibility for SHOW RANGES / crdb_internal.ranges #99618

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Mar 26, 2023

Backport 2/2 commits from #98979 on behalf of @knz.

/cc @cockroachdb/release


Requested by @mwang1026 and @nvanbenschoten .

Fixes #97861.
Fixes #99073.
Needed for #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.


Release justification: prevents regression, introduces a deprecation notice

knz added 2 commits March 19, 2023 13:03
This change introduces a new stage during planning, which upon
encountering a planning error adds additional user-facing hints to the
error payload.

We will be able to extend this over time to make suggestions on how to
enhance a query to avoid error.

As an example application, this change enhances errors about
now-removed columns in `crdb_internal.ranges`, `ranges_no_leases` and
the output of `SHOW RANGES`.

For example:
```
demo@127.0.0.1:26257/movr> select lease_holder from [show ranges from table users];
ERROR: column "lease_holder" does not exist
SQLSTATE: 42703
HINT: To list lease holder and range size details, consider SHOW RANGES WITH DETAILS.
--
There are more SHOW RANGES options. Refer to the online documentation or execute 'SHOW RANGES ??' for details.
```

```
demo@127.0.0.1:26257/movr> select database_name,table_name from crdb_internal.ranges;
ERROR: column "database_name" does not exist
SQLSTATE: 42703
DETAIL: SELECT database_name, table_name FROM crdb_internal.ranges
HINT: To list all ranges across all databases and display the database name, consider SHOW CLUSTER RANGES WITH TABLES.
--
There are more SHOW RANGES options. Refer to the online documentation or execute 'SHOW RANGES ??' for details.
```

Release note: None
…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 (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 `SHOW RANGES` can be enabled by changing the cluster
setting `sql.show_ranges_deprecated_behavior.enabled` to `false`. It
will become default in v23.2.
@blathers-crl blathers-crl bot requested review from a team as code owners March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team as a code owner March 26, 2023 19:35
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-98979 branch from b970815 to ff48287 Compare March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team as a code owner March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team as a code owner March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from a team as a code owner March 26, 2023 19:35
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-98979 branch from 84e5fb0 to 89d69ef Compare March 26, 2023 19:35
@blathers-crl blathers-crl bot requested review from herkolategan and renatolabs and removed request for a team March 26, 2023 19:35
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-98979 branch from ff48287 to e5ab27c Compare March 26, 2023 19:35
@blathers-crl blathers-crl bot requested a review from cucaroach March 26, 2023 19:35
@blathers-crl
Copy link
Author

blathers-crl bot commented Mar 26, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Mar 26, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz removed request for a team March 26, 2023 19:39
@knz knz requested review from fqazi and nvanbenschoten and removed request for a team, herkolategan, renatolabs, cucaroach, fqazi and irfansharif March 26, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants