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: fix prepared statement query summary #83673

Merged
merged 1 commit into from
Jul 12, 2022
Merged

sql: fix prepared statement query summary #83673

merged 1 commit into from
Jul 12, 2022

Conversation

ericharmeling
Copy link
Contributor

This commit sets the StmtSummary metadata field for prepared
statements to the StatementSummary value in the prepared statement
metadata for prepared statements at execution.

Fixes #79152.

Release note (bug fix): Previously, the querySummary metadata field in
the crdb_internal.statement_statistics table was inconsistent with the
query metadata field for executed prepared statements. These fields
are now consistent for prepared statements.

@ericharmeling ericharmeling requested a review from a team as a code owner June 30, 2022 18:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling ericharmeling removed the request for review from a team June 30, 2022 18:59
maryliag
maryliag suggested changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests to confirm the correct behaviour?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

@ericharmeling ericharmeling force-pushed the fix-prepared-stmt-summary branch from 07d2e6d to e8f548f Compare July 1, 2022 16:29
ericharmeling
ericharmeling commented Jul 1, 2022
View reviewed changes
Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

@ericharmeling ericharmeling requested a review from maryliag July 1, 2022 16:55
@ericharmeling ericharmeling force-pushed the fix-prepared-stmt-summary branch from e8f548f to 9bc1f3c Compare July 1, 2022 17:45
maryliag
maryliag approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! :lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 4, 2022

Build failed:

@ericharmeling ericharmeling force-pushed the fix-prepared-stmt-summary branch from 9bc1f3c to 26cc32c Compare July 5, 2022 13:31
@ericharmeling
Copy link
Contributor Author

bors r-

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 5, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 5, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build failed:

@ericharmeling
Copy link
Contributor Author

bors retry

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build failed (retrying...):

@ericharmeling ericharmeling force-pushed the fix-prepared-stmt-summary branch from 26cc32c to fde3923 Compare July 6, 2022 19:36
@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Canceled.

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 7, 2022

Build failed (retrying...):

@yuzefovich
Copy link
Member

Looks like this PR might be flaky - the merge failed twice on TestPrepareStatisticsMetadata and this PR seems related.

bors r-

@craig
Copy link
Contributor

craig bot commented Jul 7, 2022

Canceled.

@ericharmeling
Copy link
Contributor Author

ericharmeling commented Jul 7, 2022 •
edited
Loading

Looks like this PR might be flaky - the merge failed twice on TestPrepareStatisticsMetadata and this PR seems related.

😖 you're right. I actually just built master and could not repro the behavior shown in #79152 (the issue this PR fixes). It looks like something was merged in the last week that broke querySummary for prepared statements altogether. I just checked out the last commit from June 29 (02dcb38) and was able to repro. I'm going to do some manual divide-and-conquer until I locate the culprit.

@ericharmeling
Copy link
Contributor Author

It looks like something was merged in the last week that broke querySummary for prepared statements altogether.

Well this isn't entirely true. Just that the querySummary field isn't being populated for prepared statements in crdb_internal.cluster_statement_statistics (and therefore in crdb_internal.statement_statistics.metadata).

Looks like it's one of the commits in this merge: ce1b42b

@ericharmeling
Copy link
Contributor Author

The behavior that this PR seeks to fix has changed.

See #79152 (comment).

@ericharmeling
Copy link
Contributor Author

Rebasing on master to include #84170. Note that #84170 has not been backported, so we probably shouldn't backport this PR either.

@ericharmeling
sql: fix prepared statement query summary
377e6fd 
This commit sets the StmtSummary metadata field for prepared
statements to the StatementSummary value in the prepared statement
metadata for prepared statements at execution.

Fixes cockroachdb#79152.

Release note (bug fix): Previously, the querySummary metadata field in
the crdb_internal.statement_statistics table was inconsistent with the
query metadata field for executed prepared statements. These fields
are now consistent for prepared statements.
@ericharmeling ericharmeling force-pushed the fix-prepared-stmt-summary branch from fde3923 to 377e6fd Compare July 12, 2022 16:00
@ericharmeling
Copy link
Contributor Author

Looks like the test failure here (Example_demo_locality) is flaky

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 12, 2022

Build succeeded:

@craig craig bot merged commit 625ca8d into cockroachdb:master Jul 12, 2022
@maryliag
Copy link
Contributor

maryliag commented Sep 8, 2022

@ericharmeling can you backport this to 22.1?

@ericharmeling ericharmeling mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryliag maryliag maryliag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql: SQL Stats's metadata column does not report query summary for prepared statements
4 participants
@ericharmeling @cockroach-teamcity @yuzefovich @maryliag