From 39f024d6ab07f252ced2059e1744bdab887fbeae Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Fri, 12 Apr 2024 21:54:27 +0000 Subject: [PATCH] plpgsql: prevent inlining for block-exit continuation This commit prevents inlining for the continuation that transitions out of a PL/pgSQL block with an exception handler. This is necessary to ensure that the statements following the nested block are considered part of the parent block, not the nested block. Otherwise, an error thrown after the nested block might still be caught by the nested block's exception handler, which is incorrect behavior. Informs #122278 Release note: None --- .../testdata/logic_test/plpgsql_block | 34 ++++++++++ pkg/sql/opt/memo/testdata/logprops/tail-calls | 2 +- pkg/sql/opt/optbuilder/plpgsql.go | 38 +++++++---- .../opt/optbuilder/testdata/procedure_plpgsql | 16 ++--- pkg/sql/opt/optbuilder/testdata/udf_plpgsql | 68 +++++++++---------- 5 files changed, 102 insertions(+), 56 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block b/pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block index 515bfc7f9aa2..abfcb8952ab1 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block +++ b/pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block @@ -436,6 +436,40 @@ NOTICE: inner block: 1 NOTICE: inner handler: 2 NOTICE: outer handler: 3 +# Regression test for #122278 - calling f(1) should result in the second +# exception handler being triggered. +statement ok +CREATE TABLE t122278(x INT); + +statement ok +DROP FUNCTION f; +CREATE FUNCTION f(n INT) RETURNS INT AS $$ + BEGIN + BEGIN + IF n = 0 THEN + RETURN 1 // 0; + END IF; + EXCEPTION + WHEN division_by_zero THEN + RETURN (SELECT 100 + count(*) FROM t122278); + END; + RETURN 1 // 0; + EXCEPTION + WHEN division_by_zero THEN + RETURN (SELECT 200 + count(*) FROM t122278); + END +$$ LANGUAGE PLpgSQL; + +query I +SELECT f(0); +---- +100 + +query I +SELECT f(1); +---- +200 + subtest error statement ok diff --git a/pkg/sql/opt/memo/testdata/logprops/tail-calls b/pkg/sql/opt/memo/testdata/logprops/tail-calls index 14adda5af6f2..dc4f5eafe7e3 100644 --- a/pkg/sql/opt/memo/testdata/logprops/tail-calls +++ b/pkg/sql/opt/memo/testdata/logprops/tail-calls @@ -902,7 +902,7 @@ values └── body └── values └── tuple - └── udf: exception_block_5 + └── udf: nested_block_5 ├── tail-call ├── body │ └── values diff --git a/pkg/sql/opt/optbuilder/plpgsql.go b/pkg/sql/opt/optbuilder/plpgsql.go index ec9749939847..3299366d70f3 100644 --- a/pkg/sql/opt/optbuilder/plpgsql.go +++ b/pkg/sql/opt/optbuilder/plpgsql.go @@ -372,17 +372,18 @@ func (b *plpgsqlBuilder) buildBlock(astBlock *ast.Block, s *scope) *scope { } // Build the exception handler. This has to happen after building the variable // declarations, since the exception handler can reference the block's vars. - if exceptions := b.buildExceptions(astBlock); exceptions != nil { + if len(astBlock.Exceptions) > 0 { + exceptionBlock := b.buildExceptions(astBlock) + block.hasExceptionHandler = true + // There is an implicit block around the body statements, with an optional // exception handler. Note that the variable declarations are not in block // scope, and exceptions thrown during variable declaration are not caught. // - // The routine is volatile to prevent inlining. Only the block and - // variable-assignment routines need to be volatile; see the buildExceptions + // The routine is volatile to prevent inlining; see the buildExceptions // comment for details. - block.hasExceptionHandler = true - blockCon := b.makeContinuation("exception_block") - blockCon.def.ExceptionBlock = exceptions + blockCon := b.makeContinuation("nested_block") + blockCon.def.ExceptionBlock = exceptionBlock blockCon.def.Volatility = volatility.Volatile b.appendPlpgSQLStmts(&blockCon, astBlock.Body) return b.callContinuation(&blockCon, s) @@ -407,7 +408,15 @@ func (b *plpgsqlBuilder) buildPLpgSQLStatements(stmts []ast.Statement, s *scope) // For a nested block, push a continuation with the remaining statements // before calling recursively into buildBlock. The continuation will be // called when the control flow within the nested block terminates. - blockCon := b.makeContinuationWithTyp("nested_block", t.Label, continuationBlockExit) + blockCon := b.makeContinuationWithTyp("post_nested_block", t.Label, continuationBlockExit) + if len(t.Exceptions) > 0 { + // If the block has an exception handler, mark the continuation as + // volatile to prevent inlining. This is necessary to ensure that + // transitions out of a PL/pgSQL block are correctly tracked during + // execution. The transition *into* the block is marked volatile for the + // same reason; see also buildBlock and buildExceptions. + blockCon.def.Volatility = volatility.Volatile + } b.appendPlpgSQLStmts(&blockCon, stmts[i+1:]) b.pushContinuation(blockCon) return b.buildBlock(t, s) @@ -1375,13 +1384,16 @@ func (b *plpgsqlBuilder) makeRaiseFormatMessage( // cannot throw an exception, and so the "i := 2" assignment will never become // visible. // -// The block and assignment continuations must be volatile to prevent inlining. -// The presence of an exception handler does not impose restrictions on inlining -// for other continuations. +// The block entry/exit and assignment continuations for a block with an +// exception handler must be volatile to prevent inlining. The presence of an +// exception handler does not impose restrictions on inlining for other types of +// continuations. +// +// Inlining is disabled for the block-exit continuation to ensure that the +// statements following the nested block are correctly handled as part of the +// parent block. Otherwise, an error thrown from the parent block could +// incorrectly be caught by the exception handler of the nested block. func (b *plpgsqlBuilder) buildExceptions(block *ast.Block) *memo.ExceptionBlock { - if len(block.Exceptions) == 0 { - return nil - } codes := make([]pgcode.Code, 0, len(block.Exceptions)) handlers := make([]*memo.UDFDefinition, 0, len(block.Exceptions)) addHandler := func(codeStr string, handler *memo.UDFDefinition) { diff --git a/pkg/sql/opt/optbuilder/testdata/procedure_plpgsql b/pkg/sql/opt/optbuilder/testdata/procedure_plpgsql index 21e37cde0c78..46fa3bf806a5 100644 --- a/pkg/sql/opt/optbuilder/testdata/procedure_plpgsql +++ b/pkg/sql/opt/optbuilder/testdata/procedure_plpgsql @@ -368,11 +368,11 @@ call │ │ ├── const: '' │ │ └── const: '00000' │ └── project - │ ├── columns: nested_block_3:25 + │ ├── columns: post_nested_block_3:25 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: nested_block_3 [as=nested_block_3:25] + │ └── udf: post_nested_block_3 [as=post_nested_block_3:25] │ ├── tail-call │ ├── args │ │ ├── variable: x:21 @@ -502,11 +502,11 @@ call │ │ ├── const: '' │ │ └── const: '00000' │ └── project - │ ├── columns: exception_block_7:17 + │ ├── columns: nested_block_7:17 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_7 [as=exception_block_7:17] + │ └── udf: nested_block_7 [as=nested_block_7:17] │ ├── tail-call │ ├── args │ │ └── variable: x:2 @@ -532,11 +532,11 @@ call │ │ │ ├── const: 1 │ │ │ └── const: 0 │ │ └── project - │ │ ├── columns: nested_block_3:15 + │ │ ├── columns: post_nested_block_3:15 │ │ ├── values │ │ │ └── tuple │ │ └── projections - │ │ └── udf: nested_block_3 [as=nested_block_3:15] + │ │ └── udf: post_nested_block_3 [as=post_nested_block_3:15] │ │ ├── tail-call │ │ ├── args │ │ │ └── variable: x:13 @@ -581,7 +581,7 @@ call │ └── exception-handler │ └── SQLSTATE '22012' │ └── project - │ ├── columns: nested_block_3:11 + │ ├── columns: post_nested_block_3:11 │ ├── barrier │ │ ├── columns: x:10!null │ │ └── project @@ -591,7 +591,7 @@ call │ │ └── projections │ │ └── const: 100 [as=x:10] │ └── projections - │ └── udf: nested_block_3 [as=nested_block_3:11] + │ └── udf: post_nested_block_3 [as=post_nested_block_3:11] │ ├── args │ │ └── variable: x:10 │ ├── params: x:4 diff --git a/pkg/sql/opt/optbuilder/testdata/udf_plpgsql b/pkg/sql/opt/optbuilder/testdata/udf_plpgsql index 313bc9e28100..4d1e51ff1cbe 100644 --- a/pkg/sql/opt/optbuilder/testdata/udf_plpgsql +++ b/pkg/sql/opt/optbuilder/testdata/udf_plpgsql @@ -3717,13 +3717,13 @@ project └── udf: f [as=f:6] └── body └── limit - ├── columns: exception_block_7:5 + ├── columns: nested_block_7:5 ├── project - │ ├── columns: exception_block_7:5 + │ ├── columns: nested_block_7:5 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_7 [as=exception_block_7:5] + │ └── udf: nested_block_7 [as=nested_block_7:5] │ ├── body │ │ └── project │ │ ├── columns: stmt_return_8:4!null @@ -3786,13 +3786,13 @@ project ├── params: i:1 └── body └── limit - ├── columns: exception_block_3:13 + ├── columns: nested_block_3:13 ├── project - │ ├── columns: exception_block_3:13 + │ ├── columns: nested_block_3:13 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_3 [as=exception_block_3:13] + │ └── udf: nested_block_3 [as=nested_block_3:13] │ ├── args │ │ └── variable: i:1 │ ├── params: i:4 @@ -3875,13 +3875,13 @@ project ├── params: i:1 └── body └── limit - ├── columns: exception_block_5:15 + ├── columns: nested_block_5:15 ├── project - │ ├── columns: exception_block_5:15 + │ ├── columns: nested_block_5:15 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_5 [as=exception_block_5:15] + │ └── udf: nested_block_5 [as=nested_block_5:15] │ ├── args │ │ └── variable: i:1 │ ├── params: i:6 @@ -3960,13 +3960,13 @@ project └── udf: f [as=f:8] └── body └── limit - ├── columns: exception_block_7:7 + ├── columns: nested_block_7:7 ├── project - │ ├── columns: exception_block_7:7 + │ ├── columns: nested_block_7:7 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_7 [as=exception_block_7:7] + │ └── udf: nested_block_7 [as=nested_block_7:7] │ ├── body │ │ └── project │ │ ├── columns: "_stmt_raise_8":6 @@ -4045,9 +4045,9 @@ project ├── params: n:1 └── body └── limit - ├── columns: exception_block_3:9 + ├── columns: nested_block_3:9 ├── project - │ ├── columns: exception_block_3:9 + │ ├── columns: nested_block_3:9 │ ├── barrier │ │ ├── columns: i:2 │ │ └── project @@ -4059,7 +4059,7 @@ project │ │ ├── const: 100 │ │ └── variable: n:1 │ └── projections - │ └── udf: exception_block_3 [as=exception_block_3:9] + │ └── udf: nested_block_3 [as=nested_block_3:9] │ ├── args │ │ ├── variable: n:1 │ │ └── variable: i:2 @@ -4114,9 +4114,9 @@ project ├── params: i:1 j:2 k:3 └── body └── limit - ├── columns: exception_block_3:33 + ├── columns: nested_block_3:33 ├── project - │ ├── columns: exception_block_3:33 + │ ├── columns: nested_block_3:33 │ ├── barrier │ │ ├── columns: x:4!null │ │ └── project @@ -4126,7 +4126,7 @@ project │ │ └── projections │ │ └── const: 0 [as=x:4] │ └── projections - │ └── udf: exception_block_3 [as=exception_block_3:33] + │ └── udf: nested_block_3 [as=nested_block_3:33] │ ├── args │ │ ├── variable: i:1 │ │ ├── variable: j:2 @@ -4250,9 +4250,9 @@ project ├── params: i:1 └── body └── limit - ├── columns: exception_block_3:30 + ├── columns: nested_block_3:30 ├── project - │ ├── columns: exception_block_3:30 + │ ├── columns: nested_block_3:30 │ ├── barrier │ │ ├── columns: x:2 │ │ └── project @@ -4263,7 +4263,7 @@ project │ │ └── cast: INT8 [as=x:2] │ │ └── null │ └── projections - │ └── udf: exception_block_3 [as=exception_block_3:30] + │ └── udf: nested_block_3 [as=nested_block_3:30] │ ├── args │ │ ├── variable: i:1 │ │ └── variable: x:2 @@ -4443,9 +4443,9 @@ project ├── params: n:1 a:2 └── body └── limit - ├── columns: exception_block_3:44 + ├── columns: nested_block_3:44 ├── project - │ ├── columns: exception_block_3:44 + │ ├── columns: nested_block_3:44 │ ├── barrier │ │ ├── columns: x:3 i:4!null │ │ └── project @@ -4460,7 +4460,7 @@ project │ │ └── projections │ │ └── const: 0 [as=i:4] │ └── projections - │ └── udf: exception_block_3 [as=exception_block_3:44] + │ └── udf: nested_block_3 [as=nested_block_3:44] │ ├── args │ │ ├── variable: n:1 │ │ ├── variable: a:2 @@ -5494,9 +5494,9 @@ project └── udf: f [as=f:19] └── body └── limit - ├── columns: exception_block_5:18 + ├── columns: nested_block_5:18 ├── project - │ ├── columns: exception_block_5:18 + │ ├── columns: nested_block_5:18 │ ├── barrier │ │ ├── columns: curs:1!null │ │ └── project @@ -5506,7 +5506,7 @@ project │ │ └── projections │ │ └── const: 'foo' [as=curs:1] │ └── projections - │ └── udf: exception_block_5 [as=exception_block_5:18] + │ └── udf: nested_block_5 [as=nested_block_5:18] │ ├── args │ │ └── variable: curs:1 │ ├── params: curs:10 @@ -5942,13 +5942,13 @@ project └── udf: f [as=f:6] └── body └── limit - ├── columns: exception_block_7:5 + ├── columns: nested_block_7:5 ├── project - │ ├── columns: exception_block_7:5 + │ ├── columns: nested_block_7:5 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: exception_block_7 [as=exception_block_7:5] + │ └── udf: nested_block_7 [as=nested_block_7:5] │ ├── body │ │ └── project │ │ ├── columns: stmt_return_8:4!null @@ -6531,11 +6531,11 @@ project │ │ ├── const: '' │ │ └── const: '00000' │ └── project - │ ├── columns: nested_block_3:17 + │ ├── columns: post_nested_block_3:17 │ ├── values │ │ └── tuple │ └── projections - │ └── udf: nested_block_3 [as=nested_block_3:17] + │ └── udf: post_nested_block_3 [as=post_nested_block_3:17] │ ├── tail-call │ ├── args │ │ └── variable: outer_quantity:14 @@ -6751,11 +6751,11 @@ project │ │ └── null [type=unknown] │ └── subquery [type=tuple{int, unknown, decimal}] │ └── project - │ ├── columns: nested_block_6:7(tuple{int, unknown, decimal}) + │ ├── columns: post_nested_block_6:7(tuple{int, unknown, decimal}) │ ├── values │ │ └── tuple [type=tuple] │ └── projections - │ └── udf: nested_block_6 [as=nested_block_6:7, type=tuple{int, unknown, decimal}] + │ └── udf: post_nested_block_6 [as=post_nested_block_6:7, type=tuple{int, unknown, decimal}] │ └── body │ └── project │ ├── columns: stmt_if_1:5(tuple{int, unknown, decimal})