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

opt: disable GenerateParameterizedJoin when forcing custom plans #129050

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 15, 2024

opt: remove documentation of old flags

In #85935 we introduced the set flag to set session settings in
optimizer tests and a few flags that were already controlling session
settings were removed. The documentation of those old flags has been
removed.

Release note: None

opt: disable GenerateParameterizedJoin when forcing custom plans

The exploration rule GenerateParameterizedJoin is now disabled when
the plan_cache_mode session setting is set to force_custom_plan.
This prevents possible regressions in plans for this mode in the rare
case that a stable function is not folded during normalization (due to
an error encountered during folding). In most cases, the check for
placeholders and stable functions in GenerateParameterizedJoin is
sufficient for preventing the rule from firing when a generic query plan
is not being built—before optimizing a custom plan placeholders are
always replaced with constants and stable functions are usually folded
to constants.

Epic: None

Release note: None

sql: fix minor typo in plan_opt.go

Release note: None

@mgartner mgartner added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Aug 15, 2024
@mgartner mgartner requested a review from a team August 15, 2024 14:15
@mgartner mgartner requested a review from a team as a code owner August 15, 2024 14:15
@mgartner mgartner requested review from michae2 and removed request for a team August 15, 2024 14:15
@mgartner
Copy link
Collaborator Author

Only the second commit needs to be backported.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

This prevents possible regressions in plans for this mode in the rare
case that a stable function is not folded during normalization (due to
an error encountered during folding). In most cases, the check for
placeholders and stable functions in GenerateParameterizedJoin is
sufficient for preventing the rule from firing when a generic query plan
is not being built—before optimizing a custom plan placeholders are
always replaced with constants and stable functions are usually folded
to constants.

Thanks for this description!

When optimizing a custom plan, if a stable function is not folded during normalization, is it possible that we would actually want this rule to fire? Could it turn a FTS with a filter involving an unfolded stable function into a lookup join? (I'm not sure what an example would be... maybe a call to a stable UDF?)

Anyway, this :lgtm: if we want to disable the rule.

Reviewed 1 of 1 files at r1, 8 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rytaft)


pkg/sql/opt/testutils/opttester/opt_tester.go line 485 at r2 (raw file):

//
//   - generic: enables optimizations for generic query plans.
//     NOTE: This flag sets the plan_cache_mode session setting to "auto", which

Just curious: any particular reason for auto instead of force_generic_plan?

@mgartner
Copy link
Collaborator Author

When optimizing a custom plan, if a stable function is not folded during normalization, is it possible that we would actually want this rule to fire? Could it turn a FTS with a filter involving an unfolded stable function into a lookup join? (I'm not sure what an example would be... maybe a call to a stable UDF?)

Yes great point. I thought of that a bit, and I do think we would want it to fire. It could unlock optimizations that are otherwise impossible. But we do not want it to fire in the backported version of generic query plans, to reduce risk. So we'll stick with this for now, and maybe remove it soon.

@mgartner mgartner requested a review from michae2 August 15, 2024 19:57
Copy link
Collaborator Author

@mgartner mgartner 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! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @michae2, and @rytaft)


pkg/sql/opt/testutils/opttester/opt_tester.go line 485 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Just curious: any particular reason for auto instead of force_generic_plan?

Not really. Auto will become the default at some point though, so I thought I'd mimic that. When it does, we'll need to change this setting.

Copy link
Collaborator

@DrewKimball DrewKimball 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 1 of 1 files at r1, 8 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @michae2 and @rytaft)

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.

:lgtm:

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

In cockroachdb#85935 we introduced the `set` flag to set session settings in
optimizer tests and a few flags that were already controlling session
settings were removed. The documentation of those old flags has been
removed.

Release note: None
The exploration rule `GenerateParameterizedJoin` is now disabled when
the `plan_cache_mode` session setting is set to `force_custom_plan`.
This prevents possible regressions in plans for this mode in the rare
case that a stable function is not folded during normalization (due to
an error encountered during folding). In most cases, the check for
placeholders and stable functions in `GenerateParameterizedJoin` is
sufficient for preventing the rule from firing when a generic query plan
is not being built—before optimizing a custom plan placeholders are
always replaced with constants and stable functions are usually folded
to constants.

Release note: None
@mgartner mgartner force-pushed the simple-disable-parameterized-join branch from 1a8286a to 2d0ea24 Compare August 22, 2024 16:58
@mgartner
Copy link
Collaborator Author

TRTRs!

bors r+

@craig craig bot merged commit 1decd78 into cockroachdb:master Aug 22, 2024
21 of 23 checks passed
Copy link

blathers-crl bot commented Aug 22, 2024

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 bdda987 to blathers/backport-release-23.2-129050: 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.2.x failed. See errors above.


error creating merge commit from bdda987 to blathers/backport-release-24.1-129050: 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 24.1.x failed. See errors above.


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

@mgartner mgartner deleted the simple-disable-parameterized-join branch August 22, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants