Skip to content

Commit

Permalink
opt,plpgsql: subquery hoisting rules should not reorder PL/pgSQL subr…
Browse files Browse the repository at this point in the history 
…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 #120327

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.
  • Loading branch information
@DrewKimball
DrewKimball committed Mar 14, 2024
1 parent 1a66d6d commit 619bb47
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 0 deletions.
20 changes: 20 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/procedure_plpgsql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,24 @@ CALL get_rows(3);
----
NOTICE: (1,)

# Regression test for #120439 - maintain the execution ordering of PL/pgSQL
# subroutines.
subtest regression_120439

statement ok
CREATE PROCEDURE p(x INT) LANGUAGE PLpgSQL AS $$
BEGIN
IF pg_sleep(0.1) IS NOT NULL THEN
RAISE NOTICE 'foo %', x;
ELSE
SELECT x;
END IF;
END
$$;

query T noticetrace
CALL p(1);
----
NOTICE: foo 1

subtest end
35 changes: 35 additions & 0 deletions pkg/sql/opt/norm/decorrelate_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,41 @@ func (c *CustomFuncs) HasHoistableSubquery(scalar opt.ScalarExpr) bool {
}

func (c *CustomFuncs) deriveHasHoistableSubquery(scalar opt.ScalarExpr) bool {
if c.deriveHasUnhoistableExpr(scalar) {
// Some expressions disqualify subquery-hoisting entirely.
return false
}
return c.deriveHasHoistableSubqueryImpl(scalar)
}

// 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
// triggered, *all* correlated subqueries are hoisted, not just the leak-proof
// subqueries. Therefore, it is necessary for correctness to avoid hoisting
// entirely in the presence of certain expressions.
func (c *CustomFuncs) deriveHasUnhoistableExpr(expr opt.Expr) bool {
switch t := expr.(type) {
case *memo.BarrierExpr:
// An optimization barrier indicates the presence of an expression which
// cannot be reordered with other expressions.
return true
case *memo.UDFCallExpr:
if t.TailCall {
// A routine with the "tail-call" property cannot be reordered with other
// expressions, since it may then no longer be in tail-call position.
return true
}
}
for i := 0; i < expr.ChildCount(); i++ {
if c.deriveHasUnhoistableExpr(expr.Child(i)) {
return true
}
}
return false
}

func (c *CustomFuncs) deriveHasHoistableSubqueryImpl(scalar opt.ScalarExpr) bool {
switch t := scalar.(type) {
case *memo.SubqueryExpr:
return !t.Input.Relational().OuterCols.Empty()
Expand Down
125 changes: 125 additions & 0 deletions pkg/sql/opt/norm/testdata/rules/routine
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# Regression test for #120439 - don't allow subquery hoisting that violates
# PL/pgSQL subroutine ordering.
exec-ddl
CREATE OR REPLACE PROCEDURE p(x INT) AS $$
BEGIN
IF pg_sleep(0.1) IS NOT NULL THEN
RAISE NOTICE 'foo %', x;
ELSE
SELECT x;
END IF;
END
$$ LANGUAGE PLpgSQL;
----

# The subquery hoisting rules shouldn't pull subroutine tail calls out of a
# CASE statement.
norm expect-not=HoistValuesSubquery format=show-scalars
CALL p(1);
----
call
├── volatile
└── procedure: p
├── args
│ └── const: 1
├── params: x:1
└── body
└── values
├── columns: stmt_if_5:12
├── outer: (1)
├── cardinality: [1 - 1]
├── volatile
├── key: ()
├── fd: ()-->(12)
└── tuple
└── case
├── true
├── when
│ ├── is-not
│ │ ├── function: pg_sleep
│ │ │ └── const: 0.1
│ │ └── null
│ └── subquery
│ └── values
│ ├── columns: "_stmt_raise_2":7
│ ├── outer: (1)
│ ├── cardinality: [1 - 1]
│ ├── volatile
│ ├── key: ()
│ ├── fd: ()-->(7)
│ └── tuple
│ └── udf: _stmt_raise_2
│ ├── args
│ │ └── variable: x:1
│ ├── params: x:4
│ └── body
│ ├── values
│ │ ├── columns: stmt_raise_3:5
│ │ ├── outer: (4)
│ │ ├── cardinality: [1 - 1]
│ │ ├── volatile
│ │ ├── key: ()
│ │ ├── fd: ()-->(5)
│ │ └── tuple
│ │ └── function: crdb_internal.plpgsql_raise
│ │ ├── const: 'NOTICE'
│ │ ├── concat
│ │ │ ├── concat
│ │ │ │ ├── const: 'foo '
│ │ │ │ └── coalesce
│ │ │ │ ├── cast: STRING
│ │ │ │ │ └── variable: x:4
│ │ │ │ └── const: '<NULL>'
│ │ │ └── const: ''
│ │ ├── const: ''
│ │ ├── const: ''
│ │ └── const: '00000'
│ └── values
│ ├── columns: stmt_if_1:6
│ ├── cardinality: [1 - 1]
│ ├── key: ()
│ ├── fd: ()-->(6)
│ └── tuple
│ └── subquery
│ └── values
│ ├── columns: "_implicit_return":3
│ ├── cardinality: [1 - 1]
│ ├── key: ()
│ ├── fd: ()-->(3)
│ └── tuple
│ └── null
└── subquery
└── values
├── columns: "_stmt_exec_4":11
├── outer: (1)
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(11)
└── tuple
└── udf: _stmt_exec_4
├── args
│ └── variable: x:1
├── params: x:8
└── body
├── values
│ ├── columns: x:9
│ ├── outer: (8)
│ ├── cardinality: [1 - 1]
│ ├── key: ()
│ ├── fd: ()-->(9)
│ └── tuple
│ └── variable: x:8
└── values
├── columns: stmt_if_1:10
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(10)
└── tuple
└── subquery
└── values
├── columns: "_implicit_return":3
├── cardinality: [1 - 1]
├── key: ()
├── fd: ()-->(3)
└── tuple
└── null

0 comments on commit 619bb47

Please sign in to comment.