opt,plpgsql: subquery hoisting rules should not reorder PL/pgSQL subroutines #120451
Conversation
DrewKimball
added
the
backport-23.2.x
|
This change is |
|
This is effectively a more targeted fix to #97432, which I hope to backport. It only applies to PL/pgSQL routines, so it should make a very safe backport. |
There was a problem hiding this comment.
The explanation sounds good to me, but I'll defer to Marcus for approval.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)
-- commits line 20 at r1:
nit: the linked number is a PR - is this intentional?
…outines Due to cockroachdb#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 cockroachdb#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 cockroachdb#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.
DrewKimball
force-pushed
the
unhoistable
branch
from
619bb47
to
15b2fb4
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: the linked number is a PR - is this intentional?
Not intentional, good catch.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/opt/norm/decorrelate_funcs.go line 57 at r2 (raw file):
// deriveHasUnhoistableExpr checks for expressions within the given scalar // expression which cannot be hoisted. This is necessary beyond existing // volatility checks because of #97432: when a subquery-hoisting rule is
You have a fix for #97432, so does that make this PR obsolete?
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/sql/opt/norm/decorrelate_funcs.go line 57 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
You have a fix for #97432, so does that make this PR obsolete?
I do expect that to fix this, but I want to backport this change. The other fix probably isn't backportable unless we disable it by default.
|
bors r+ |
|
Build failed: |
|
Looks like a flake. bors retry |
Ok. So we'll revert this change on master then? |
We could. I think I'd rather spend some time thinking about updating our optimizer restrictions on volatility first, though. Does that sound ok? |
Yes, absolutely, no need to rush it. Consider creating an issue to track this potential clean-up, if you think we might forget about it. |
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
Barrierexpression 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
BarrierExpror aUDFCallwithTailCall = 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
IFstatement with one leak-proof branch, and one branch with side effects.