-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Improve partial stats description #143283
base: master
Are you sure you want to change the base?
Improve partial stats description #143283
Conversation
Previously, the job description for AUTO CREATE PARTIAL STATS defaulted to a raw SQL description, making it harder to read and difficult to distinguish these jobs in system.jobs. This was inadequate because users need clear job descriptions to differentiate between full and partial automatic statistics updates, especially for debugging or monitoring job execution. To address this, this patch updates the job description to include plain English and explicitly indicate that the job is for automatic partial statistics collection, improving clarity in system.jobs. Fixes cockroachdb#140054. Release note (sql change): The job description for AUTO CREATE PARTIAL STATS now clearly indicates its purpose in plain English, improving system.jobs visibility and debugging.
Previously, the job description for AUTO CREATE PARTIAL STATS defaulted to a raw SQL description, making it harder to read and difficult to distinguish these jobs in system.jobs. This was inadequate because users need clear job descriptions to differentiate between full and partial automatic statistics updates, especially for debugging or monitoring job execution. To address this, this patch updates the job description to include plain English and explicitly indicate that the job is for automatic partial statistics collection, improving clarity in system.jobs. Fixes cockroachdb#140054. Release note (sql change): The job description for AUTO CREATE PARTIAL STATS now clearly indicates its purpose in plain English, improving system.jobs visibility and debugging.
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @veronicavaldez! This is definitely on the right track, but there are a few issues with the test that I've called out below. Please also squash your commits.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @veronicavaldez)
pkg/sql/create_stats_test.go
line 247 at r2 (raw file):
sqlRunner := sqlutils.MakeSQLRunner(sqlDB) // Enable both automatic statistics collection and partial stats collection
nit: end comments with a period (here and below).
pkg/sql/create_stats_test.go
line 249 at r2 (raw file):
// Enable both automatic statistics collection and partial stats collection sqlRunner.Exec(t, `SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true;`) sqlRunner.Exec(t, `SET CLUSTER SETTING sql.stats.automatic_partial_collection.enabled = true;`)
Since you are manually creating stats below, I don't think you need to enable these settings. It might be better to disable the settings to prevent flakiness in the test.
pkg/sql/create_stats_test.go
line 254 at r2 (raw file):
sqlRunner.Exec(t, `CREATE TABLE test (id INT PRIMARY KEY, value INT);`) // Insert more data to increase likelihood of triggering partial stats
You don't need to increase the likelihood of triggering partial stats, since you are explicitly collecting partial stats below.
It's not a bad idea to insert data into the table, but I don't think it's strictly necessary given that you're manually collecting stats. I'd either remove this INSERT
altogether, or update the comment.
pkg/sql/create_stats_test.go
line 283 at r2 (raw file):
WHERE (job_type = 'AUTO CREATE STATS' OR job_type = 'AUTO CREATE PARTIAL STATS') AND description LIKE '%test%' LIMIT 1`).Scan(&description)
This will be flaky since it might return a row for AUTO CREATE STATS
which will have the incorrect description. You should only select job type AUTO CREATE PARTIAL STATS
in order to avoid failing the check below (ditto above in the SucceedsSoon
loop).
(Technically this might work if you disable the automatic stats collection cluster setting and only use ANALYZE
, but it would be misleading.)
Previously, the job description for AUTO CREATE PARTIAL STATS defaulted to a raw SQL description, making it harder to read and difficult to distinguish these jobs in system.jobs.
This was inadequate because users need clear job descriptions to differentiate between full and partial automatic statistics updates, especially for debugging or monitoring job execution.
To address this, this patch updates the job description to include plain English and explicitly indicate that the job is for automatic partial statistics collection, improving clarity in system.jobs.
Fixes #140054.
Release note (sql change): The job description for AUTO CREATE PARTIAL STATS now clearly indicates its purpose in plain English, improving system.jobs visibility and debugging.