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: assorted generic query plan improvements #143085

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mgartner
Copy link
Collaborator

sql/logictest: add match option to query directive

The match(<regexp>) option is now supported for the query test
directive. Any rows that do not have a column value with a string
representation matching <regexp> will be excluded in the result.

This option is useful for EXPLAIN queries which cannot be included in
square-bracket subqueries which could normally be used to filter result
rows, e.g., SELECT * FROM [EXPLAIN ...] WHERE ....

Release note: None

sql/logictest: simplify generic query plan tests

The generic test file has been simplified using the match option of
the query directive.

Release note: None

sql: reset generic and custom plan costs if either is stale

If either of the prebuilt generic or custom memos are stale, the costs
for both are cleared and the decision to use a custom or generic plan
for a prepared statement is reset. This ensures that we do not compare
costs of generic and custom plans based on different stats or schema
versions.

Fixes #132968
Informs #127826

Release note: None

sql: consider cost flags when choosing generic or custom plans

Cost flags are now considered when choosing between generic or custom
query plans.

Fixes #127826

Release note: None

opt: move Cost tests to memo package

Unit tests for the Cost type have been moved from the memo_test
package to the memo package.

Release note: None

sql: do not prefer generic plans with more full scans than custom plans

Under plan_cache_mode=auto, a generic query plan is not chosen if it
has more full scans than any of the 5 custom plans previously created.
This heuristic will help avoid choosing suboptimal generic query plans.
If a generic query plan has more full scans than some custom plan, then
the cost of re-optimization is likely worth it to try to avoid the full
scan.

Informs #137018
Informs #136975

Release note: None

The `match(<regexp>)` option is now supported for the `query` test
directive. Any rows that do not have a column value with a string
representation matching `<regexp>` will be excluded in the result.

This option is useful for `EXPLAIN` queries which cannot be included in
square-bracket subqueries which could normally be used to filter result
rows, e.g., `SELECT * FROM [EXPLAIN ...] WHERE ...`.

Release note: None
The `generic` test file has been simplified using the `match` option of
the `query` directive.

Release note: None
If either of the prebuilt generic or custom memos are stale, the costs
for both are cleared and the decision to use a custom or generic plan
for a prepared statement is reset. This ensures that we do not compare
costs of generic and custom plans based on different stats or schema
versions.

Fixes cockroachdb#132968
Informs cockroachdb#127826

Release note: None
@mgartner mgartner requested a review from a team March 18, 2025 19:43
@mgartner mgartner requested a review from a team as a code owner March 18, 2025 19:43
@mgartner mgartner requested review from rytaft and removed request for a team March 18, 2025 19:43
Copy link

blathers-crl bot commented Mar 18, 2025

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Cost flags are now considered when choosing between generic or custom
query plans.

Fixes cockroachdb#127826

Release note: None
Unit tests for the `Cost` type have been moved from the `memo_test`
package to the `memo` package.

Release note: None
Under `plan_cache_mode=auto`, a generic query plan is not chosen if it
has more full scans than any of the 5 custom plans previously created.
This heuristic will help avoid choosing suboptimal generic query plans.
If a generic query plan has more full scans than some custom plan, then
the cost of re-optimization is likely worth it to try to avoid the full
scan.

Informs cockroachdb#137018
Informs cockroachdb#136975

Release note: None
@mgartner mgartner force-pushed the assorted-generic-improvements branch from 7ac9d0a to 44aae72 Compare March 19, 2025 15:16
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Nice fixes!

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 4 of 4 files at r4, 1 of 1 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)


-- commits line 18 at r2:
Nice simplification of the test!


pkg/sql/prepared_stmt.go line 182 at r4 (raw file):

//
// If the cost flags have not yielded a result, then the generic plan is optimal
// if its cost value is less than the average cost of the custom plans.

Feels like you should have some sort of test that will fail if we add new cost flags to make sure that you've considered them all.

Alternatively, you could consider making use of the CostFlags.Less function, which I think should probably do the right thing.


pkg/sql/logictest/testdata/logic_test/generic line 370 at r6 (raw file):


# The generic query plan is not chosen because it has a full table scan, while
# the custom plan does not.

Do you have any tests for the case where both have full table scans, but one has more than the other?


pkg/sql/prepared_stmt.go line 184 at r3 (raw file):

}

// Reset clears any previous set costs.

nit: previous -> previously

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants