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

sql: statement diagnostics never finish for (SQL-level) prepared statements #66048

Closed
michae2 opened this issue Jun 3, 2021 · 0 comments · Fixed by #66074
Closed

sql: statement diagnostics never finish for (SQL-level) prepared statements #66048

michae2 opened this issue Jun 3, 2021 · 0 comments · Fixed by #66074
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@michae2
Copy link
Collaborator

michae2 commented Jun 3, 2021

While investigating a performance problem, @yuzefovich discovered that statement diagnostics requests are never completed for statements prepared and executed with the SQL PREPARE and EXECUTE commands. (Note that these SQL commands are different from the prepare and execute messages in the protocol used by most ORMs and applications.)

To reproduce:

  1. In a SQL shell, execute a simple prepared statement:
    CREATE TABLE foo (f INT PRIMARY KEY);
    PREPARE sel AS SELECT * FROM foo WHERE f = $1;
    EXECUTE sel(0);
  2. In the UI, find the SELECT * FROM foo WHERE f = $1 statement in the UI under "Statements".
  3. Click Diagnostics > Activate Diagnostics
  4. In a new SQL session, prepare and execute it again:
    PREPARE sel AS SELECT * FROM foo WHERE f = $1;
    EXECUTE sel(0);
  5. In the UI, the diagnostics status will still be WAITING.

This was on 21.1.1.

@michae2 michae2 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jun 3, 2021
@RaduBerinde RaduBerinde changed the title Statement diagnostics never finish for (SQL-level) prepared statements sql: statement diagnostics never finish for (SQL-level) prepared statements Jun 4, 2021
craig bot pushed a commit that referenced this issue Jun 4, 2021
65142: sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor r=postamar a=ajwerner

The `ResolvedSchema` was a blemish on the descriptor resolution APIs. It made
it very hard to create a reasonable abstraction for descriptor resolution
injection. This commit creates new implementations of `SchemaDescriptor` to
represent virtual, public, and temporary schemas.

There is likely more cleanup that could be done along the way to make the
different kinds of schemas more uniform, but that is left for later or never.

Relates to #64089.

Release note: None

65780: sql: Add unit tests for planning inside the new schema changer r=ajwerner a=fqazi

Previously, the new schema changer did not have any unit
tests covering the planning capability. This was inadequate,
because we had no way of detect if plans were regressing with
changes or enhancements. To address this, this patch adds
basic tests to see if the operators/dependencies for a given
command are sane.

Release note: None

66051: logcrash: update non-release Sentry URL r=mgartner a=mgartner

The documentation for creating Sentry reports in non-release builds
directed developers to set the COCKROACH_CRASH_REPORTS env var to an
incorrect URL. Sentry reports were not created when using this URL. The
documentation has been updated to refer to the correct URL.

Release note: None

66070: sql: fix very slow TestSchemaChangeRetryOnVersionChange r=fqazi a=ajwerner

See [here](https://teamcity.cockroachdb.com/project.html?projectId=Cockroach_UnitTests&testNameId=6532733632782929224&tab=testDetails).
This test was taking 10 minutes every time. This was happening because of the
change to wait for the version's leases to expire in #63725.

After:
```
--- PASS: TestSchemaChangeRetryOnVersionChange (1.70s)
```

Release note: None

66074: sql: fix statement diagnostics for EXECUTE r=RaduBerinde a=RaduBerinde

Previously, prepared statements ran through the EXECUTE statements
could not trigger collection of statement diagnostics. This is because
we consider the fingerprint of the EXECUTE statement instead of the
one from the prepared statement.

The fix is to move up the special handling code in the executor, and
replace the AST and fingerprint before setting up the instrumentation
helper.

Release note (bug fix): queries ran through the EXECUTE statement can
now generate statement diagnostic bundles as expected.

Fixes #66048.

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in d3cbcfd Jun 4, 2021
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jun 4, 2021
Previously, prepared statements ran through the EXECUTE statements
could not trigger collection of statement diagnostics. This is because
we consider the fingerprint of the EXECUTE statement instead of the
one from the prepared statement.

The fix is to move up the special handling code in the executor, and
replace the AST and fingerprint before setting up the instrumentation
helper.

Release note (bug fix): queries ran through the EXECUTE statement can
now generate statement diagnostic bundles as expected.

Fixes cockroachdb#66048.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants