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: EXPLAIN ANALYZE (DEBUG) not supported for non-system tenants #70931

Closed
rytaft opened this issue Sep 30, 2021 · 1 comment · Fixed by #71680
Closed

sql: EXPLAIN ANALYZE (DEBUG) not supported for non-system tenants #70931

rytaft opened this issue Sep 30, 2021 · 1 comment · Fixed by #71680
Labels
A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
SQL Queries (OBSOLETE) Multi-tenant

Comments

@rytaft
Copy link
Collaborator

rytaft commented Sep 30, 2021

Non-system tenants may still wish to collect a statement bundle for debugging purposes using EXPLAIN ANALYZE (DEBUG).
This is not yet possible.
@rytaft rytaft added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) X-anchored-telemetry The issue number is anchored by telemetry references. A-multitenancy Related to multi-tenancy labels Sep 30, 2021
@rytaft rytaft mentioned this issue Sep 30, 2021
Closed
@rytaft rytaft added this to Triage in SQL Queries via automation Sep 30, 2021
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Sep 30, 2021
rytaft added a commit to rytaft/cockroach that referenced this issue Sep 30, 2021
@rytaft
sql: disallow non-system tenants from running EXPLAIN ANALYZE (DEBUG)
cb0ff8e 
Until we can support EXPLAIN ANALYZE (DEBUG) correctly for non-system
tenants, this command is disallowed.

Fixes cockroachdb#70770
Informs cockroachdb#70931

Release note (sql change): EXPLAIN ANALYZE (DEBUG) now returns an error
for non-system tenants, since we cannot yet support it correctly.
@rytaft rytaft mentioned this issue Sep 30, 2021
Merged
@RaduBerinde
Copy link
Member

The infrastructure for collecting the data in the bundle should work, the problem is really just getting the data out to the tenant.

There are some potential solutions here:

  • write the bundle to S3 or some other blob storage
  • devise a SQL query that returns the encoded bundle chunks and a shell script that uses cockroach sql to get that data then decode it.
  • somehow get the bundle data to Prometheus (?)

rytaft added a commit to rytaft/cockroach that referenced this issue Sep 30, 2021
@rytaft
sql: disallow non-system tenants from running EXPLAIN ANALYZE (DEBUG)
1fd0617 
Until we can support EXPLAIN ANALYZE (DEBUG) correctly for non-system
tenants, this command is disallowed.

Fixes cockroachdb#70770
Informs cockroachdb#70931

Release note (sql change): EXPLAIN ANALYZE (DEBUG) now returns an error
for non-system tenants, since we cannot yet support it correctly.
craig bot pushed a commit that referenced this issue Sep 30, 2021
@rytaft
Merge #70932
71c74c9 
70932: sql: disallow non-system tenants from running EXPLAIN ANALYZE (DEBUG) r=rytaft a=rytaft

Until we can support `EXPLAIN ANALYZE (DEBUG)` correctly for non-system
tenants, this command is disallowed.

Fixes #70770
Informs #70931

Release note (sql change): `EXPLAIN ANALYZE (DEBUG)` now returns an error
for non-system tenants, since we cannot yet support it correctly.

Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this issue Sep 30, 2021
@rytaft
sql: disallow non-system tenants from running EXPLAIN ANALYZE (DEBUG)
acb7dfc 
Until we can support EXPLAIN ANALYZE (DEBUG) correctly for non-system
tenants, this command is disallowed.

Fixes #70770
Informs #70931

Release note (sql change): EXPLAIN ANALYZE (DEBUG) now returns an error
for non-system tenants, since we cannot yet support it correctly.
@blathers-crl blathers-crl bot mentioned this issue Sep 30, 2021
Merged
@RaduBerinde RaduBerinde moved this from Triage to Backlog in SQL Queries Oct 5, 2021
andy-kimball pushed a commit that referenced this issue Oct 6, 2021
@rytaft @andy-kimball
sql: disallow non-system tenants from running EXPLAIN ANALYZE (DEBUG)
5aaded2 
Until we can support EXPLAIN ANALYZE (DEBUG) correctly for non-system
tenants, this command is disallowed.

Fixes #70770
Informs #70931

Release note (sql change): EXPLAIN ANALYZE (DEBUG) now returns an error
for non-system tenants, since we cannot yet support it correctly.
@kevin-v-ngo kevin-v-ngo mentioned this issue Oct 12, 2021
Open
@RaduBerinde RaduBerinde mentioned this issue Oct 18, 2021
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 18, 2021
@RaduBerinde
cli: add \statement-diag SQL shell command
6a9e1b8 
This commit adds a `\statement-diag` command to the SQL shell which
can be used to list and download statement bundles directly from the
shell.

This command should be useful for multitenant (where the possibilities
of downloading the bundle are more limited), as well as running demo
with a `make buildshort` binary.

The output of `EXPLAIN ANALYZE (DEBUG)` includes the exact syntax.

Informs cockroachdb#70931.

Release note (cli change): the SQL shell now supports a
`\statement-diag` command for listing and downloading statement
diagnostics bundles.
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 18, 2021
@RaduBerinde
sql: enable EXPLAIN ANALYZE (DEBUG) for tenants
0c4b99c 
EXPLAIN ANALYZE (DEBUG) was disabled for tenants because the returned URL
was unusable.

We can now download bundles through the SQL shell, so we can reenable
EXPLAIN ANALYZE (DEBUG). For non-system tenants, we hide the URL and
only show the SQL shell and cli options.

As part of this change, we also add tenant support to the interactive
cli test infrastructure and we implement the `--background` and
`--pid-file` flags for `mt start-sql`.

Fixes cockroachdb#70931.

Release note (sql change): EXPLAIN ANALYZE (DEBUG) can now be used by
tenants.
@ajstorm ajstorm added this to Planned in (OBSOLETE) Multi-tenant Oct 25, 2021
@ajstorm ajstorm moved this from Planned to Triage in (OBSOLETE) Multi-tenant Oct 25, 2021
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 25, 2021
@RaduBerinde
cli: add \statement-diag SQL shell command
db5ec49 
This commit adds a `\statement-diag` command to the SQL shell which
can be used to list and download statement bundles directly from the
shell.

This command should be useful for multitenant (where the possibilities
of downloading the bundle are more limited), as well as running demo
with a `make buildshort` binary.

The output of `EXPLAIN ANALYZE (DEBUG)` includes the exact syntax.

Informs cockroachdb#70931.

Release note (cli change): the SQL shell now supports a
`\statement-diag` command for listing and downloading statement
diagnostics bundles.
@RaduBerinde RaduBerinde moved this from Triage to 22.1 Requirement in (OBSOLETE) Multi-tenant Oct 25, 2021
craig bot pushed a commit that referenced this issue Oct 25, 2021
@bananabrick @yuzefovich
@jsoref @otan @RaduBerinde
Merge #70224 #70239 #71642 #71653 #71680
c5ca3a6 
70224: server: share the table cache among stores r=bananabrick a=bananabrick

Possible benefits of sharing the table cache are outlined
here: cockroachdb/pebble#1178.

This PR creates a table cache which can be shared among many
stores.

Release note: None

70239: sql: remove the interleaved logic from fetchers r=yuzefovich a=yuzefovich

**row: simplify fetcher to only handle a single table**

Previously, `row.Fetcher` could handle multiple tables at the same time.
This was needed to support the interleaved tables efficiently, but since
the interleaved tables are no more, we can simplify the fetcher now.

There are more things that can be removed from the fetcher about
handling of the interleaved tables, but I chose to defer them to a later
commit. This commit's main focus is simplifying the fetcher to work
under the assumption that it will be fetching only from a single table.

Addresses: #69150

Release note: None

**colbuilder: remove the check for index join on the interleaved index**

Interleaved indexes are no more.

Release note: None

**sql: remove the interleaved logic from fetchers**

This commit removes the remaining bits of handling the interleaved
tables from both row fetcher and cFetcher. It also removes a test that
verifies that we can read from the interleaved table restored from
a backup.

Release note: None

71642: misc: correct spelling mistakes r=irfansharif a=jsoref

This goal of this PR is to be limited to just code comments as suggested in:
#71464 (comment)

71653: tree: fix ambiguity with enum overloads r=rafiss a=otan

Resolves #71446 

Release note (sql change): Previously, certain enum builtins or
operators required an explicit enum cast. This has been reduced in some
cases.

71680: cli: add \statement-diag SQL shell command r=RaduBerinde a=RaduBerinde

This PR adds a `\statement-diag` command to the SQL shell used to list and download bundles, and reenables `EXPLAIN ANALYZE (DEBUG)` for tenants.

#### cli: extract statement diagnostics code

This commit extracts the part of the statment-diag code that interacts
with the system tables and moves it to sqlclientconn.

Release note: None

#### cli: improve statement-diag download

Minor improvement to `statement-diag download`: make the filename
optional, for consistency with the SQL shell equivalent.

Release note: None

#### cli: simplify invalid syntax paths in SQL shell

This change simplifies most invalid syntax paths while improving the
error message in a few cases.

Release note: None

#### cli: add \statement-diag SQL shell command

This commit adds a `\statement-diag` command to the SQL shell which
can be used to list and download statement bundles directly from the
shell.

This command should be useful for multitenant (where the possibilities
of downloading the bundle are more limited), as well as running demo
with a `make buildshort` binary.

The output of `EXPLAIN ANALYZE (DEBUG)` includes the exact syntax.

Informs #70931.

Release note (cli change): the SQL shell now supports a
`\statement-diag` command for listing and downloading statement
diagnostics bundles.

#### sql: enable EXPLAIN ANALYZE (DEBUG) for tenants

EXPLAIN ANALYZE (DEBUG) was disabled for tenants because the returned URL
was unusable.

We can now download bundles through the SQL shell, so we can reenable
EXPLAIN ANALYZE (DEBUG). For non-system tenants, we hide the URL and
only show the SQL shell and cli options.

As part of this change, we also add tenant support to the interactive
cli test infrastructure and we implement the `--background` and
`--pid-file` flags for `mt start-sql`.

Fixes #70931.

Release note (sql change): EXPLAIN ANALYZE (DEBUG) can now be used by
tenants.

Co-authored-by: Arjun Nair <nair@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Josh Soref <jsoref@users.noreply.github.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@craig craig bot closed this as completed in 87f79da Oct 25, 2021
SQL Queries automation moved this from Backlog to Done Oct 25, 2021
(OBSOLETE) Multi-tenant automation moved this from 22.1 Requirement to Done Oct 25, 2021
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 26, 2021
@RaduBerinde
cli: add \statement-diag SQL shell command
c356ba8 
This commit adds a `\statement-diag` command to the SQL shell which
can be used to list and download statement bundles directly from the
shell.

This command should be useful for multitenant (where the possibilities
of downloading the bundle are more limited), as well as running demo
with a `make buildshort` binary.

The output of `EXPLAIN ANALYZE (DEBUG)` includes the exact syntax.

Informs cockroachdb#70931.

Release note (cli change): the SQL shell now supports a
`\statement-diag` command for listing and downloading statement
diagnostics bundles.
@RaduBerinde RaduBerinde mentioned this issue Oct 26, 2021
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 26, 2021
@RaduBerinde
sql: enable EXPLAIN ANALYZE (DEBUG) for tenants
5775d7c 
EXPLAIN ANALYZE (DEBUG) was disabled for tenants because the returned URL
was unusable.

We can now download bundles through the SQL shell, so we can reenable
EXPLAIN ANALYZE (DEBUG). For non-system tenants, we hide the URL and
only show the SQL shell and cli options.

As part of this change, we also add tenant support to the interactive
cli test infrastructure and we implement the `--background` and
`--pid-file` flags for `mt start-sql`.

Fixes cockroachdb#70931.

Release note (sql change): EXPLAIN ANALYZE (DEBUG) can now be used by
tenants.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
(OBSOLETE) Multi-tenant
Done
SQL Queries
Archived in project
SQL Queries
Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cli: add \statement-diag SQL shell command RaduBerinde/cockroach
2 participants
@rytaft @RaduBerinde