Skip to content

Commit ab895ff

Browse files
craig[bot]DrewKimball
craig[bot]
and
DrewKimball
committed
Merge #156966
156966: plpgsql: move barrier after outer-join for SQL stmt with INTO clause r=DrewKimball a=DrewKimball PL/pgSQL routines use barrier expressions to prevent optimizations that would remove side effecting expressions from the query plan. Previously, we would add a barrier below the outer join used to ensure at least one row returned by a SQL statement with an INTO clause. However, this wasn't enough, since after some inlining and projection push-down into the join, the join could be eliminated. This could result in a side-effecting SQL statement being dropped if its target variable(s) weren't referenced. This commit fixes the bug by moving the barrier above the outer-join. This prevents optimizations (like join elimination) that rely on proving that the columns from the null-extended side of the join are not needed. Fixes #147269 Release note (bug fix): Fixed a bug existing since SQL statements with INTO clause were introduced for PL/pgSQL routines in v23.2. The bug could cause a SQL statement with side effects (e.g. INSERT) to be dropped if none of the target variables from the INTO clause were referenced. Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
2 parents d9b2fc2 + b706af5 commit ab895ff

File tree

4 files changed

+154
-77
lines changed

4 files changed

+154
-77
lines changed

‎pkg/sql/opt/memo/testdata/logprops/tail-calls‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -958,22 +958,22 @@ values
958958
├── params: x
959959
└── body
960960
└── project
961-
├── left-join (cross)
962-
│ ├── values
963-
│ │ └── tuple
964-
│ ├── barrier
965-
│ │ └── limit
966-
│ │ ├── project-set
967-
│ │ │ ├── values
968-
│ │ │ │ └── tuple
969-
│ │ │ └── zip
970-
│ │ │ └── udf: nested
971-
│ │ │ └── body
972-
│ │ │ └── values
973-
│ │ │ └── tuple
974-
│ │ │ └── const: 1
975-
│ │ └── const: 1
976-
│ └── filters (true)
961+
├── barrier
962+
│ └── left-join (cross)
963+
│ ├── values
964+
│ │ └── tuple
965+
│ ├── limit
966+
│ │ ├── project-set
967+
│ │ │ ├── values
968+
│ │ │ │ └── tuple
969+
│ │ │ └── zip
970+
│ │ │ └── udf: nested
971+
│ │ │ └── body
972+
│ │ │ └── values
973+
│ │ │ └── tuple
974+
│ │ │ └── const: 1
975+
│ │ └── const: 1
976+
│ └── filters (true)
977977
└── projections
978978
└── variable: nested
979979

‎pkg/sql/opt/optbuilder/plpgsql.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,13 +1000,6 @@ func (b *plpgsqlBuilder) buildPLpgSQLStatements(stmts []ast.Statement, s *scope)
10001000
stmtScope.makeOrderingChoice(),
10011001
)
10021002

1003-
// Add an optimization barrier in case the projected variables are never
1004-
// referenced again, to prevent column-pruning rules from dropping the
1005-
// side effects of executing the SELECT ... INTO statement.
1006-
if stmtScope.expr.Relational().VolatilitySet.HasVolatile() {
1007-
b.ob.addBarrier(stmtScope)
1008-
}
1009-
10101003
if strict {
10111004
// Check that the expression produces exactly one row.
10121005
b.addOneRowCheck(stmtScope)
@@ -1022,6 +1015,14 @@ func (b *plpgsqlBuilder) buildPLpgSQLStatements(stmts []ast.Statement, s *scope)
10221015
)
10231016
}
10241017

1018+
// Add an optimization barrier in case the projected variables are never
1019+
// referenced again, to prevent column-pruning and join-elimination rules
1020+
// from dropping the side effects of executing the SELECT ... INTO
1021+
// statement.
1022+
if stmtScope.expr.Relational().VolatilitySet.HasVolatile() {
1023+
b.ob.addBarrier(stmtScope)
1024+
}
1025+
10251026
// Step 2: build the INTO statement into a continuation routine that calls
10261027
// the previously built continuation.
10271028
intoScope := b.buildInto(stmtScope, t.Target)

‎pkg/sql/opt/optbuilder/testdata/procedure_plpgsql‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -813,34 +813,34 @@ call
813813
│ │ │ │ ├── columns: "_stmt_exec_ret_2":17
814814
│ │ │ │ ├── project
815815
│ │ │ │ │ ├── columns: foo:16
816-
│ │ │ │ │ ├── right-join (cross)
816+
│ │ │ │ │ ├── barrier
817817
│ │ │ │ │ │ ├── columns: i:7
818-
│ │ │ │ │ │ ├── barrier
819-
│ │ │ │ │ │ │ ├── columns: i:7!null
820-
│ │ │ │ │ │ │ └── limit
821-
│ │ │ │ │ │ │ ├── columns: i:7!null
822-
│ │ │ │ │ │ │ ├── project
823-
│ │ │ │ │ │ │ │ ├── columns: i:7!null
824-
│ │ │ │ │ │ │ │ └── insert t
825-
│ │ │ │ │ │ │ │ ├── columns: k:6!null i:7!null s:8!null
826-
│ │ │ │ │ │ │ │ ├── insert-mapping:
827-
│ │ │ │ │ │ │ │ │ ├── column1:11 => k:6
828-
│ │ │ │ │ │ │ │ │ ├── column2:12 => i:7
829-
│ │ │ │ │ │ │ │ │ └── column3:13 => s:8
830-
│ │ │ │ │ │ │ │ ├── return-mapping:
831-
│ │ │ │ │ │ │ │ │ ├── column1:11 => k:6
832-
│ │ │ │ │ │ │ │ │ ├── column2:12 => i:7
833-
│ │ │ │ │ │ │ │ │ └── column3:13 => s:8
834-
│ │ │ │ │ │ │ │ └── values
835-
│ │ │ │ │ │ │ │ ├── columns: column1:11!null column2:12!null column3:13!null
836-
│ │ │ │ │ │ │ │ └── tuple
837-
│ │ │ │ │ │ │ │ ├── const: 1
838-
│ │ │ │ │ │ │ │ ├── const: 2
839-
│ │ │ │ │ │ │ │ └── const: 'foo'
840-
│ │ │ │ │ │ │ └── const: 1
841-
│ │ │ │ │ │ ├── values
842-
│ │ │ │ │ │ │ └── tuple
843-
│ │ │ │ │ │ └── filters (true)
818+
│ │ │ │ │ │ └── right-join (cross)
819+
│ │ │ │ │ │ ├── columns: i:7
820+
│ │ │ │ │ │ ├── limit
821+
│ │ │ │ │ │ │ ├── columns: i:7!null
822+
│ │ │ │ │ │ │ ├── project
823+
│ │ │ │ │ │ │ │ ├── columns: i:7!null
824+
│ │ │ │ │ │ │ │ └── insert t
825+
│ │ │ │ │ │ │ │ ├── columns: k:6!null i:7!null s:8!null
826+
│ │ │ │ │ │ │ │ ├── insert-mapping:
827+
│ │ │ │ │ │ │ │ │ ├── column1:11 => k:6
828+
│ │ │ │ │ │ │ │ │ ├── column2:12 => i:7
829+
│ │ │ │ │ │ │ │ │ └── column3:13 => s:8
830+
│ │ │ │ │ │ │ │ ├── return-mapping:
831+
│ │ │ │ │ │ │ │ │ ├── column1:11 => k:6
832+
│ │ │ │ │ │ │ │ │ ├── column2:12 => i:7
833+
│ │ │ │ │ │ │ │ │ └── column3:13 => s:8
834+
│ │ │ │ │ │ │ │ └── values
835+
│ │ │ │ │ │ │ │ ├── columns: column1:11!null column2:12!null column3:13!null
836+
│ │ │ │ │ │ │ │ └── tuple
837+
│ │ │ │ │ │ │ │ ├── const: 1
838+
│ │ │ │ │ │ │ │ ├── const: 2
839+
│ │ │ │ │ │ │ │ └── const: 'foo'
840+
│ │ │ │ │ │ │ └── const: 1
841+
│ │ │ │ │ │ ├── values
842+
│ │ │ │ │ │ │ └── tuple
843+
│ │ │ │ │ │ └── filters (true)
844844
│ │ │ │ │ └── projections
845845
│ │ │ │ │ └── variable: i:7 [as=foo:16]
846846
│ │ │ │ └── projections

0 commit comments

Comments
 (0)