-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
release-23.2: opt,plpgsql: subquery hoisting rules should not reorder PL/pgSQL subroutines #120742
release-23.2: opt,plpgsql: subquery hoisting rules should not reorder PL/pgSQL subroutines #120742
Conversation
674f773
to
dd92697
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
Reminder: it has been 3 weeks please merge or close your backport! |
…outines Due to #97432, it is possible for subquery-hoisting decorrelation rules to hoist a volatile subquery from a CASE expression. This can cause a query to display side effects which were meant to be gated behind a conditional expression, or else were meant to occur in a different order. This is a problem for PL/pgSQL, which relies on expressions being executed in a certain order. While #115826 added a `Barrier` expression to prevent rules from changing execution order, this doesn't work for hoisting rules that traverse an entire operator subtree, instead of relying on match-and-replace patterns. This commit makes a targeted fix for PL/pgSQL routines by preventing subquery-hoisting rules from matching if a scalar expression contains a `BarrierExpr` or a `UDFCall` with `TailCall = true`. Either of these conditions indicates that changing execution order would cause incorrect results. Fixes #120439 Release note (bug fix): Fixed a bug introduced in v23.2 that could cause a PL/pgSQL routine to return incorrect results when there was at least one parameter, and an `IF` statement with one leak-proof branch, and one branch with side effects.
dd92697
to
1199468
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @rafiss, and @yuzefovich)
Friendly ping @DrewKimball - we good to merge? |
TFYR |
Backport 1/1 commits from #120451 on behalf of @DrewKimball.
/cc @cockroachdb/release
Due to #97432, it is possible for subquery-hoisting decorrelation rules to hoist a volatile subquery from a CASE expression. This can cause a query to display side effects which were meant to be gated behind a conditional expression, or else were meant to occur in a different order. This is a problem for PL/pgSQL, which relies on expressions being executed in a certain order. While #115826 added a
Barrier
expression to prevent rules from changing execution order, this doesn't work for hoisting rules that traverse an entire operator subtree, instead of relying on match-and-replace patterns.This commit makes a targeted fix for PL/pgSQL routines by preventing subquery-hoisting rules from matching if a scalar expression contains a
BarrierExpr
or aUDFCall
withTailCall = true
. Either of these conditions indicates that changing execution order would cause incorrect results.Fixes #120439
Release note (bug fix): Fixed a bug introduced in v23.2 that could cause a PL/pgSQL routine to return incorrect results when there was at least one parameter, and an
IF
statement with one leak-proof branch, and one branch with side effects.Release justification: targeted fix for PL/pgSQL routines