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

builtins: create function to aggregate aggregated stmt metadata #105351

Merged

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Jun 22, 2023

server: prefer string concat over str format where possible

A number of places in the combined stats api were using string
format where a string concatenation would have worked.

Release note: None

Epic: none

server: format sql activity queries

Reformat queries for consistency and readability.

Epic: none

Release note: None

builtins: create function to aggregate aggregated stmt metadata

The statement activity table used to cache aggregated stmt stats for the sql activity page stores aggregated metadata. Previously we used the same query as the one used for system.statement_statistics, which stores unaggregated metadata, on the activity table. Using the aggregate function for unaggregated metadata on aggregated metadata was nto valid, leading to incorrect JSON fields being produced. This commit introduces the builtin crdb_internal.combine_aggregated_stmt_metadata which can be used to correctly aggregate the metadata column on the stmt activity table.

Fixes: #103895

Release note (bug fix): On the UI, selecting a database filter from the filters menu in the sql activity page should function as expected. This fixes a preivous bug where the filter would break and not show any results when the results were retrieved from the stmt activity table instead of the persisted table.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the fix-stmt-activity-metadata-aggregation branch 2 times, most recently from 8574c07 to ee8a191 Compare June 22, 2023 20:55
@xinhaoz xinhaoz marked this pull request as ready for review June 26, 2023 14:55
@xinhaoz xinhaoz requested review from a team June 26, 2023 14:56
@xinhaoz xinhaoz requested a review from a team as a code owner June 26, 2023 14:56
@xinhaoz xinhaoz force-pushed the fix-stmt-activity-metadata-aggregation branch from ee8a191 to dad7cc3 Compare June 29, 2023 03:11
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.

Your PR has 3 commits, so on the PR description add more information about the changes on the other 2 commits.

:lgtm:

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 5 of 5 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/server/combined_statement_stats.go line 1513 at r3 (raw file):

	// We will have 1 open iterator at a time. For the deferred close operation, we will
	// only close the iterator if there were no errors creating the iterator. Closing an
	// iterator that returned an error on creation will cause a nil pointer deref.

nit: defer

@maryliag maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 4, 2023
@xinhaoz xinhaoz force-pushed the fix-stmt-activity-metadata-aggregation branch 3 times, most recently from 28a87a6 to d85958f Compare July 6, 2023 14:57
A number of places in the combined stats api were using string
format where a string concatenation would have worked.

Release note: None

Epic: none
Reformat queries for consistency and readability.

Epic: none

Release note: None
The statement activity table used to cache aggregated stmt stats for the
sql activity page stores aggregated metadata. Previously we used the
same query as the one used for system.statement_statistics, which stores
unaggregated metadata, on the activity table. Using the aggregate function
for unaggregated metadata on aggregated metadata was nto valid, leading to
incorrect JSON fields being produced. This commit introduces the builtin
`crdb_internal.combine_aggregated_stmt_metadata` which can be used to
correctly aggregate the  metadata column on the stmt activity table.

Fixes: cockroachdb#103895

Release note (bug fix): On the UI, selecting a database filter
from the filters menu in the sql activity page should function
as expected. This fixes a preivous bug where the filter would
break and not show any results when the results were retrieved
from the stmt activity table instead of the persisted table.
@xinhaoz xinhaoz force-pushed the fix-stmt-activity-metadata-aggregation branch from d85958f to 3339656 Compare July 6, 2023 17:23
@xinhaoz
Copy link
Member Author

xinhaoz commented Jul 6, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jul 6, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 6, 2023

Build succeeded:

@craig craig bot merged commit 938e484 into cockroachdb:master Jul 6, 2023
2 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Jul 6, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 68c6390 to blathers/backport-release-23.1-105351: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
3 participants