-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
admission: auto "CREATE STATS" integration #118369
base: master
Are you sure you want to change the base?
admission: auto "CREATE STATS" integration #118369
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
36dc3f4
to
1999da7
Compare
1999da7
to
78ed64a
Compare
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)
pkg/sql/stats/automatic_stats.go
line 901 at r1 (raw file):
qosLevel := sessiondatapb.SystemLow log.Infof(ctx, "automatically executing %q", stmt) _ /* rows */, err := r.ex.ExecEx(
Hope you don't mind a drive-by question: does this change propagate all the way to TableReaders used by the distsql stats plan? My intuition says no ... I'd guess that this change is applied to the "CREATE STATISTICS ..." query which kicks off a separate execution plan, and that plan doesn't use the new QoS level.
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/stats/automatic_stats.go
line 901 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Hope you don't mind a drive-by question: does this change propagate all the way to TableReaders used by the distsql stats plan? My intuition says no ... I'd guess that this change is applied to the "CREATE STATISTICS ..." query which kicks off a separate execution plan, and that plan doesn't use the new QoS level.
I see, thanks for the callout. So the CREATE STATISTICS
in this automatic_stats.go
is not what is actually run? I am a little confused between this and the DistSQLPlanner.planAndRunCreateStats
.
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)
pkg/sql/stats/automatic_stats.go
line 901 at r1 (raw file):
Previously, aadityasondhi (Aaditya Sondhi) wrote…
I see, thanks for the callout. So the
CREATE STATISTICS
in thisautomatic_stats.go
is not what is actually run? I am a little confused between this and theDistSQLPlanner.planAndRunCreateStats
.
CREATE STATISTICS
is kinda a delegated query, i.e. executing this statement results in effectively running another internal query (for which the distsql plan is constructed in planAndRunCreateStats
.
More precisely, CREATE STATISTICS
statements creates an AST with createStatsNode
in it, then createStatsNode.startExec
creates a job record, then - some time later - the jobs registry adopts this job, and when the job gets "resumed", then planAndRunCreateStats
is invoked which actually collects table stats.
No description provided.