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 for custom query plans #128409

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Aug 6, 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 for custom query plans

The exploration rule GenerateParameterizedJoin is now disabled when a
custom query plan is being built. The mechanism for toggling this rule
is using either Optimizer.Optimize() which disables the rule or
Optimizer.OptimizeGeneric() which enables the rule. Note that the rule
is not toggled by the plan_cache_mode session setting, because the
auto mode may build custom or generic query plans. Also, changing the
plan_cache_mode session setting does not make memos stale as other
session settings do, because generic memos are kept separate from the
unoptimized memos reused to build custom plans.

Epic: None

Release note: None

opt: add "Generic" optgen tag

Optgen now supports the "Generic" tag and the IsGeneric() method has
been added to RuleName which returns true if the rule has the tag. The
new method is used to disable rules that optimize generic query plans
when custom plans are being built.

Release note: None

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 a
custom query plan is being built. The mechanism for toggling this rule
is using either `Optimizer.Optimize()` which disables the rule or
`Optimizer.OptimizeGeneric()` which enables the rule. Note that the rule
is not toggled by the `plan_cache_mode` session setting, because the
`auto` mode may build custom or generic query plans. Also, changing the
`plan_cache_mode` session setting does not make memos stale as other
session settings do, because generic memos are kept separate from the
unoptimized memos reused to build custom plans.

Release note: None
Optgen now supports the "Generic" tag and the `IsGeneric()` method has
been added to `RuleName` which returns true if the rule has the tag. The
new method is used to disable rules that optimize generic query plans
when custom plans are being built.

Release note: None
@mgartner mgartner requested a review from a team August 6, 2024 19:49
@mgartner mgartner requested a review from a team as a code owner August 6, 2024 19:49
@mgartner mgartner requested review from michae2, rytaft and DrewKimball and removed request for a team August 6, 2024 19:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@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 6, 2024
@mgartner
Copy link
Collaborator Author

mgartner commented Aug 6, 2024

I'll backport only the second commit to 23.2 and 24.1. See the conversation in #128085 (review).

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.

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


pkg/sql/opt/xform/optimizer.go line 244 at r2 (raw file):

	originalMatchedRule := o.matchedRule
	defer func() { o.matchedRule = originalMatchedRule }()
	o.matchedRule = func(r opt.RuleName) bool {

[nit] I'm open to pushback, but I'd prefer to make generic rule checks part of the generated rule code (probably here and here). We could use the Generic tag to do that. For the backportable portion, we could just make the check part of the GenerateParameterizedJoin rule as a custom func. What do you think?

@rytaft
Copy link
Collaborator

rytaft commented Aug 6, 2024

pkg/sql/opt/xform/optimizer.go line 244 at r2 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] I'm open to pushback, but I'd prefer to make generic rule checks part of the generated rule code (probably here and here). We could use the Generic tag to do that. For the backportable portion, we could just make the check part of the GenerateParameterizedJoin rule as a custom func. What do you think?

Any reason you can't just check the value of plan_cache_mode inside GenerateParameterizedJoin? I realize that will miss certain cases for auto, but I feel like it will be a smaller and more easily backportable change. I guess another option is to do the small change for the backport and do something like this for 24.2 and beyond.

@mgartner
Copy link
Collaborator Author

mgartner commented Aug 7, 2024

pkg/sql/opt/xform/optimizer.go line 244 at r2 (raw file):

[nit] I'm open to pushback, but I'd prefer to make generic rule checks part of the generated rule code (probably here and here). We could use the Generic tag to do that. For the backportable portion, we could just make the check part of the GenerateParameterizedJoin rule as a custom func. What do you think?

I agree with avoiding use of o.matchedRule like this. But the question is how to plumb a genericRulesEnabled bool into that part of xform? I didn't love the idea of setting a bool field on Optimizer or Memo on this because it seemed tricky to reason about when to reset this value given that both can be reused in some cases. I suppose it might be find to set on the Memo because a generic memo will remain generic and never be reoptimized?

Any reason you can't just check the value of plan_cache_mode inside GenerateParameterizedJoin? I realize that will miss certain cases for auto, but I feel like it will be a smaller and more easily backportable change. I guess another option is to do the small change for the backport and do something like this for 24.2 and beyond.

Ya, we could just check plan_cache_mode=custom for the backport. The rule may still fire in some cases when plan_cache_mode=auto when we don't want or need it to. That's probably an acceptable risk though.

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.

Why doesn't the HasPlaceholdersOrStableExprs check at the beginning of the rule suffice for disabling the rule when optimizing custom plans? It seems like that check will always be false when optimizing a custom plan. But maybe I'm missing something?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @rytaft)

@mgartner
Copy link
Collaborator Author

Why doesn't the HasPlaceholdersOrStableExprs check at the beginning of the rule suffice for disabling the rule when optimizing custom plans? It seems like that check will always be false when optimizing a custom plan. But maybe I'm missing something?

It mostly does. However, if we fail to fold a stable function during normalization, then we'll leave the unfolded function in the memo. In those rare cases, GenerateParameterizedJoin could fire during exploration even if plan_cache_mode=force_custom_plan. Maybe that's not a case worth worrying about?

In any case, I think the simplest and safest thing to do is what @rytaft suggests and add a simple check for plan_cache_mode. This will be sufficient to prevent the backports from causing any regressions.

@mgartner
Copy link
Collaborator Author

Please review the alternative PR #129050.

@mgartner mgartner closed this Aug 15, 2024
@mgartner mgartner deleted the disable-generate-parameterized-join branch August 15, 2024 14:16
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