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.

Sign up for GitHub

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

Jump to bottom

sql: SHOW RANGES does not show start/end keys #80906

Closed
erikgrinaker opened this issue May 3, 2022 · 2 comments · Fixed by #93644
Closed

sql: SHOW RANGES does not show start/end keys #80906

erikgrinaker opened this issue May 3, 2022 · 2 comments · Fixed by #93644
Assignees
@knz
Labels
A-kv-observability C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented May 3, 2022 •
edited by exalate-issue-sync bot
Loading

Describe the problem

The SHOW RANGES command appears to not emit the start_key and end_key of the ranges. They are always NULL.

To Reproduce

root@localhost:26257/defaultdb> SHOW RANGES FROM DATABASE system;
            table_name            | start_key | end_key | range_id | range_size_mb | lease_holder |               lease_holder_locality               |  replicas   |                                                                                                                          replica_localities
----------------------------------+-----------+---------+----------+---------------+--------------+---------------------------------------------------+-------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  comments                        | NULL      | NULL    |       20 |             0 |            4 | cloud=gce,region=europe-west1,zone=europe-west1-b | {1,2,3,4,5} | {"cloud=gce,region=europe-west1,zone=europe-west1-b","cloud=gce,region=europe-west1,zone=europe-west1-b","cloud=gce,region=europe-west1,zone=europe-west1-b","cloud=gce,region=europe-west1,zone=europe-west1-b","cloud=gce,region=europe-west1,zone=europe-west1-b"}

This is because it strips the table prefix from the keys, but the key bounds in a single-range table are just e.g. /Table/50 and /Table/51, so the entire key is stripped, yielding NULL.

This is really misleading. We should show the complete key. Discussed in https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1660947554266819.

Additional data / screenshots

Seen on both master, 21.2.9 and 21.2.10.

Jira issue: CRDB-15457

Epic CRDB-22701
@erikgrinaker erikgrinaker added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-observability Related to observability of the SQL layer T-sql-observability labels May 3, 2022
@erikgrinaker
Copy link
Contributor Author

Seems like it's been this way at least as far back as 21.1.0. It strips the table prefix from the keys and only shows the user key. So e.g. /Table/50/foo would be truncated to /foo. However, single-range tables have bounds that fall on the table key (e.g. /Table/50) and will therefore always be truncated to NULL. Seems unfortunate, but I guess changing it would be a breaking change.

@erikgrinaker
Copy link
Contributor Author

Recently discussed in https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1660947554266819. Considered misleading, so I'm reopening.

@erikgrinaker erikgrinaker changed the title sql: SHOW RANGES no longer shows start/end keys sql: SHOW RANGES does not show start/end keys Aug 20, 2022
@erikgrinaker erikgrinaker reopened this Aug 20, 2022
@maryliag maryliag removed A-sql-observability Related to observability of the SQL layer T-sql-observability labels Aug 22, 2022
@knz knz mentioned this issue Dec 13, 2022
Merged
craig bot pushed a commit that referenced this issue Dec 14, 2022
@andreimatei @ecwall
@knz @ajwerner @otan
Merge #93153 #93325 #93354 #93545 #93557 #93563 #93618
a30fb14 
93153: rttanalysis: don't count leasing the database desc r=andreimatei a=andreimatei

A bunch of rtt-analysis tests were counting a request for leasing the database descriptor. This is not interesting. This patch makes the test framework lease it first through a "USE" statement.

The number of KV requests required for leasing is currently mis-counted. We count 1, but in reality it's 4. A different patch will correct the miscounting that, at which point that would be too significant for the tests.

Release note: None
Epic: None

93325: multitenant: retain range splits after TRUNCATE for secondary tenants r=knz a=ecwall

Fixes #69499
Fixes #82944

Existing split points are preserved after a TRUNCATE statement is executed by a secondary tenant.

Release note: None

93354: tracing: disallow children of sterile span with different Tracer r=andreimatei a=andreimatei

Before this patch, creating a "child" of a sterile span with a different Tracer than the one used to create the sterile span was tolerated - on the argument that sterile spans don't actually get children (the would-be child span is created as a root), so the arguments for not allowing a children to be created with different tracers don't apply. At the same time, creating a child of a noop span with a different Tracer than the noop span's Tracer was documented to not be allowed. In practice, it was, because the code was confused [1].

This patch disallows creating children of sterile spans with a different tracer, for consistency with all the other spans. The patch also makes it a panic for the children of noop spans to be created with a different Tracer.

This is all meant as a cleanup / code simplification.

[1] WithParent(sp) meant to treat sterile spans differently than noop spans but it was using sp.IsSterile(), which returns true for both. As such, it was unintentionally returning an empty parent option. startSpanGeneric() meant to check the tracer of parent noop spans, but it was failing to actually do so because it was going through the opts.Parent.empty().

Release note: None
Epic: None

93545: sql: make SHOW RANGES FOR TABLE include all indexes r=ajwerner a=knz

Informs #80906.
Fixes #93546.
Fixes #82995.

Release note (backward-incompatible change): `SHOW RANGES FOR TABLE`
now includes rows for all indexes that support the table. Prior to
this change, `SHOW RANGES FOR TABLE foo` was an alias for `SHOW RANGES
FOR INDEX foo@primary`. This was causing confusion, as it would miss
data for secondary indexes. It is still possible to filter to just the
primary index using `SHOW RANGES FOR INDEX foo@primary`.

The statement output now also includes the index name.

93557: syntheticprivilegecache: scan all privileges at startup  r=ajwerner a=ajwerner

#### syntheticprivilegecache: move caching logic out of sql
This is a pure refactor to move the logic for caching synthetic privileges
from the sql package. This will make it easier to add features later.

#### syntheticprivilegecache: scan all privileges at startup 


Fixes #93182

This commit attempts to alleviate the pain caused by synthetic virtual table
privileges introduced in 22.2. We need to fetch privileges for virtual tables
to determine whether the user has access. This is done lazily. However, when a
user attempts to read a virtual table like pg_class or run SHOW TABLES it will
force the privileges to be determined for each virtual table (of which there
are 290 at the time of writing). This sequential process can be somewhat slow
in a single region cluster and will be very slow in an MR cluster.

This patch attempts to somewhat alleviate this pain by scanning the table
eagerly during server startup.

Release note (performance improvement): In 22.2 we introduced privileges on
virtual tables (system catalogs like pg_catalog, information_schema, and
crdb_internal). A problem with this new feature is that we now must fetch those
privileges into a cache before we can use those tables or determine their
visibility in other system catalogs. This process used to occur on-demand, when
the privilege was needed. Now we'll fetch these privileges eagerly during
startup to mitigate the latency when accessing pg_catalog right after the
server boots up.

93563: pgwire: handle decoding Geometry/Geography in binary r=rafiss a=otan

Resolves #81066
Resolves #93352

Release note (bug fix): Previously, CockroachDB would error when receiving Geometry/Geography using binary parameters. This is now resolved.

93618: cli,cliccl: move some mt commands to cliccl r=ajwerner a=ajwerner

Part of #91714

Epic: none

Release note: None

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
@knz knz mentioned this issue Dec 14, 2022
Merged
@exalate-issue-sync exalate-issue-sync bot added T-kv KV Team and removed T-kv-observability labels Dec 22, 2022
@craig craig bot closed this as completed in 6b72f70 Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@knz knz

Labels
A-kv-observability C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: better SHOW RANGES and changes to crdb_internal.ranges{,_no_leases} knz/cockroach
4 participants
@jlinder @knz @erikgrinaker @maryliag