From c96de42c4b5f5d7bf785265cf26836326e28c6b6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 16 Oct 2024 17:36:29 -0400 Subject: [PATCH] Further refine _SPI_execute_plan's rule for atomic execution. Commit 2dc1deaea turns out to have been still a brick shy of a load, because CALL statements executing within a plpgsql exception block could still pass the wrong snapshot to stable functions within the CALL's argument list. That happened because standard_ProcessUtility forces isAtomicContext to true if IsTransactionBlock is true, which it always will be inside a subtransaction. Then ExecuteCallStmt would think it does not need to push a new snapshot --- but _SPI_execute_plan didn't do so either, since it thought it was in nonatomic mode. The best fix for this seems to be for _SPI_execute_plan to operate in atomic execution mode if IsSubTransaction() is true, even when the SPI context as a whole is non-atomic. This makes _SPI_execute_plan have the same rules about when non-atomic execution is allowed as _SPI_commit/_SPI_rollback have about when COMMIT/ROLLBACK are allowed, which seems appropriately symmetric. (If anyone ever tries to allow COMMIT/ROLLBACK inside a subtransaction, this would all need to be rethought ... but I'm unconvinced that such a thing could be logically consistent at all.) For further consistency, also check IsSubTransaction() in SPI_inside_nonatomic_context. That does not matter for its one present-day caller StartTransaction, which can't be reached inside a subtransaction. But if any other callers ever arise, they'd presumably want this definition. Per bug #18656 from Alexander Alehin. Back-patch to all supported branches, like previous fixes in this area. Discussion: https://postgr.es/m/18656-cade1780866ef66c@postgresql.org --- src/backend/executor/spi.c | 14 ++++++++++---- src/pl/plpgsql/src/expected/plpgsql_call.out | 20 ++++++++++++++++++++ src/pl/plpgsql/src/sql/plpgsql_call.sql | 17 +++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 90d9834576..2fb2e73604 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -334,13 +334,13 @@ _SPI_rollback(bool chain) MemoryContext oldcontext = CurrentMemoryContext; SavedTransactionCharacteristics savetc; - /* see under SPI_commit() */ + /* see comments in _SPI_commit() */ if (_SPI_current->atomic) ereport(ERROR, (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION), errmsg("invalid transaction termination"))); - /* see under SPI_commit() */ + /* see comments in _SPI_commit() */ if (IsSubTransaction()) ereport(ERROR, (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION), @@ -582,8 +582,11 @@ SPI_inside_nonatomic_context(void) { if (_SPI_current == NULL) return false; /* not in any SPI context at all */ + /* these tests must match _SPI_commit's opinion of what's atomic: */ if (_SPI_current->atomic) return false; /* it's atomic (ie function not procedure) */ + if (IsSubTransaction()) + return false; /* if within subtransaction, it's atomic */ return true; } @@ -2411,9 +2414,12 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, /* * We allow nonatomic behavior only if options->allow_nonatomic is set - * *and* the SPI_OPT_NONATOMIC flag was given when connecting. + * *and* the SPI_OPT_NONATOMIC flag was given when connecting and we are + * not inside a subtransaction. The latter two tests match whether + * _SPI_commit() would allow a commit; see there for more commentary. */ - allow_nonatomic = options->allow_nonatomic && !_SPI_current->atomic; + allow_nonatomic = options->allow_nonatomic && + !_SPI_current->atomic && !IsSubTransaction(); /* * Setup error traceback support for ereport() diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out index 0a63b1d44e..ea7107dca0 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_call.out +++ b/src/pl/plpgsql/src/expected/plpgsql_call.out @@ -597,6 +597,26 @@ NOTICE: f_get_x(1) NOTICE: f_print_x(1) NOTICE: f_get_x(2) NOTICE: f_print_x(2) +-- test in non-atomic context, except exception block is locally atomic +DO $$ +BEGIN + BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + EXCEPTION WHEN division_by_zero THEN + RAISE NOTICE '%', SQLERRM; + END; + ROLLBACK; +END +$$; +NOTICE: f_get_x(1) +NOTICE: f_print_x(1) +NOTICE: f_get_x(2) +NOTICE: f_print_x(2) -- test in atomic context BEGIN; DO $$ diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql index 4cbda0382e..08c1659ef1 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_call.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql @@ -557,6 +557,23 @@ BEGIN END $$; +-- test in non-atomic context, except exception block is locally atomic +DO $$ +BEGIN + BEGIN + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + UPDATE t_test SET x = x + 1; + RAISE NOTICE 'f_get_x(%)', f_get_x(); + CALL f_print_x(f_get_x()); + EXCEPTION WHEN division_by_zero THEN + RAISE NOTICE '%', SQLERRM; + END; + ROLLBACK; +END +$$; + -- test in atomic context BEGIN;