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: add computed columns to crdb_internal persisted sql stats views #99240

Merged

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Mar 22, 2023

Follows #99042, #99148.

This commit adds the following computed columns to both crdb_internal.statement_statistics_persisted and crdb_internal.transaction_statistics_persisted views:
-execution_count
-service_latency
-cpu_sql_nanos
-contention_time
-total_estimated_execution_time
-p99_latency

These computed columns are indexed in the system tables from which these views are created.

Epic: None

Release note: The crdb_internal.statement_statistics_persisted and crdb_internal.transaction_statistics_persisted views now include computed columns, to optimize the performance of frequently-queried expressions.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling

This comment was marked as outdated.

@ericharmeling ericharmeling marked this pull request as ready for review March 22, 2023 17:26
@ericharmeling ericharmeling requested review from a team March 22, 2023 17:26
@ericharmeling ericharmeling requested a review from a team as a code owner March 22, 2023 17:26
@ericharmeling ericharmeling requested review from michae2 and removed request for a team and michae2 March 22, 2023 17:26
@ericharmeling ericharmeling added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 22, 2023
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.

:lgtm:

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

Follows cockroachdb#99042, cockroachdb#99148.

This commit adds the following computed columns to both
crdb_internal.statement_statistics_persisted and
crdb_internal.transaction_statistics_persisted views:
-execution_count
-service_latency
-cpu_sql_nanos
-contention_time
-total_estimated_execution_time
-p99_latency

These computed columns are indexed in the system
tables from which these views are created.

Epic: None

Release note: The crdb_internal.statement_statistics_persisted
and crdb_internal.transaction_statistics_persisted views now
include computed columns, to optimize the performance of
frequently-queried expressions.
@ericharmeling ericharmeling force-pushed the add-computed-to-sql-stats-views branch from 52521d6 to 1a03549 Compare March 22, 2023 23:53
@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 23, 2023

Build failed:

@ericharmeling
Copy link
Contributor Author

bors retry

@craig
Copy link
Contributor

craig bot commented Mar 23, 2023

Build succeeded:

@rytaft
Copy link
Collaborator

rytaft commented Mar 24, 2023

pkg/sql/opt/exec/execbuilder/testdata/sql_statistics_persisted line 4 at r3 (raw file):


statement ok
CREATE STATISTICS system_statement_stats FROM system.statement_statistics

Sorry for the late drive-by, but what was the goal with collecting statistics here? The table is empty, so this test is not going to be representative of what the plan would look like with a lot of rows. I would suggest either removing this call to CREATE STATISTICS (in which case the optimizer will default to assuming 1000 rows), or use ALTER TABLE ... INJECT STATISTICS and inject stats corresponding to what the table looks like with, say, 1 million rows.

Also, whenever you call CREATE STATISTICS in an execbuilder test like this, you'll need a retry on the next query that uses that table to avoid flakes (I already made that change in another PR, but FYI for the future). The retry is not needed for INJECT STATISTICS, however, because that causes the cache to be invalidated immediately. So if you remove this line or change it to use INJECT STATISTICS you can remove the retry directives I added.

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/opt/exec/execbuilder/testdata/sql_statistics_persisted line 4 at r3 (raw file):

what was the goal with collecting statistics here?

I want to make sure we are using the new secondary indexes on the underlying system._statistics tables for this specific query. Without statistics, the EXPLAIN plan output has scans of the primary index for the subqueries, rather than the secondary indexes on the system table.

I would suggest either removing this call to CREATE STATISTICS (in which case the optimizer will default to assuming 1000 rows), or use ALTER TABLE ... INJECT STATISTICS and inject stats corresponding to what the table looks like with, say, 1 million rows.

Also, whenever you call CREATE STATISTICS in an execbuilder test like this, you'll need a retry on the next query that uses that table to avoid flakes (I already made that change in another PR, but FYI for the future). The retry is not needed for INJECT STATISTICS, however, because that causes the cache to be invalidated immediately. So if you remove this line or change it to use INJECT STATISTICS you can remove the retry directives I added.

I'll open a new PR to use INJECT STATISTICS here instead of CREATE STATISTICS and remove the retry directives. Then I'll try to backport that (I just merged the backport of this PR 😬). 1 million rows is the maximum that these tables can hold, so that would work well.

Thank you!

craig bot pushed a commit that referenced this pull request Mar 27, 2023
99509: bazci: protect against `nil` target r=healthy-pod a=rickystewart

Closes #98861.

Epic: none

Release note: None

99532: sql: inject stats into TestExecBuild_sql_statistics_persisted testdata r=ericharmeling a=ericharmeling

Background thread: #99240 (comment)

Part of #99399.

This commit replaces the `CREATE STATISTICS` statements in the `TestExecBuild_sql_statistics_persisted` testdata with `ALTER TABLE ... INJECT STATISTICS` and removes the retry directives added to deflake the test in #99447.

Epic: none

Release note: None

99642: serverccl: skip TestServerControllerHTTP under deadlock r=dhartunian a=knz

Fixes #98046.
Release note: None

99660: workload/tpcc: disable check 3.3.2.11 after workload r=renatolabs a=nvanbenschoten

Fixes #99619.
Fixes #99594.
Fixes #99603.
Fixes #99604.
Fixes #99617.
Fixes #99616.
Fixes #99613.
Fixes #99611.
Fixes #99604.
Fixes #99603.
Fixes #99600.
Fixes #99599.
Fixes #99598.
Fixes #99596.
Fixes #99595.
Fixes #99594.

This commit disables TPC-C's consistency check 3.3.2.11 (added in #99542) after the workload has run. The check asserts a relationship between the number of rows in the "order" table and rows in the "new_order" table. Rows are inserted into these tables transactional by the NewOrder transaction. However, only rows in the "new_order" table are deleted by the Delivery transaction. Consequently, the consistency condition will fail after the first Delivery transaction is run by the workload.

See #99542 (comment) for more details.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
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
Development

Successfully merging this pull request may close these issues.

4 participants