-
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
plpgsql: decorrelation rules can re-arrange PL/pgSQL subroutines #120439
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-sql-queries
SQL Queries Team
Comments
DrewKimball
added
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-sql-queries
SQL Queries Team
labels
Mar 13, 2024
craig bot
pushed a commit
that referenced
this issue
Mar 20, 2024
120451: opt,plpgsql: subquery hoisting rules should not reorder PL/pgSQL subroutines r=DrewKimball a=DrewKimball 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. Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
blathers-crl bot
pushed a commit
that referenced
this issue
Mar 20, 2024
…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.
DrewKimball
added a commit
that referenced
this issue
Apr 10, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
T-sql-queries
SQL Queries Team
Due to #97432, decorrelation rules can sometimes hoist side-effecting subqueries. This is problematic for PL/pgSQL, which relies on evaluation order for correctness, e.g. for making the tail-call optimization valid. Here's an example where a RAISE statement fails to execute because of this issue:
pg_sleep
returnsTrue
, so the RAISE statement should have executed here. Instead,HoistValuesSubquery
fires, ignoring the guarantees provided byCaseExpr
, and hoists the subquery that contains the RAISE. Since the sub-routine is no longer in tail-call position, TCO becomes invalid and the side effect is lost. #120327 will fix this behavior, but we need a more targeted, backportable solution as well.Jira issue: CRDB-36688
The text was updated successfully, but these errors were encountered: