Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d18ee6f

Browse filesBrowse files
tglsfdctglsfdc
committed
Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.
COMMIT/ROLLBACK necessarily destroys all snapshots within the session. The original implementation of intra-procedure transactions just cavalierly did that, ignoring the fact that this left us executing in a rather different environment than normal. In particular, it turns out that handling of toasted datums depends rather critically on there being an outer ActiveSnapshot: otherwise, when SPI or the core executor pop whatever snapshot they used and return, it's unsafe to dereference any toasted datums that may appear in the query result. It's possible to demonstrate "no known snapshots" and "missing chunk number N for toast value" errors as a result of this oversight. Historically this outer snapshot has been held by the Portal code, and that seems like a good plan to preserve. So add infrastructure to pquery.c to allow re-establishing the Portal-owned snapshot if it's not there anymore, and add enough bookkeeping support that we can tell whether it is or not. We can't, however, just re-establish the Portal snapshot as part of COMMIT/ROLLBACK. As in normal transaction start, acquiring the first snapshot should wait until after SET and LOCK commands. Hence, teach spi.c about doing this at the right time. (Note that this patch doesn't fix the problem for any PLs that try to run intra-procedure transactions without using SPI to execute SQL commands.) This makes SPI's no_snapshots parameter rather a misnomer, so in HEAD, rename that to allow_nonatomic. replication/logical/worker.c also needs some fixes, because it wasn't careful to hold a snapshot open around AFTER trigger execution. That code doesn't use a Portal, which I suspect someday we're gonna have to fix. But for now, just rearrange the order of operations. This includes back-patching the recent addition of finish_estate() to centralize the cleanup logic there. This also back-patches commit 2ecfeda into v13, to improve the test coverage for worker.c (it was that test that exposed that worker.c's snapshot management is wrong). Per bug #15990 from Andreas Wicht. Back-patch to v11 where intra-procedure COMMIT was added. Discussion: https://postgr.es/m/15990-eee2ac466b11293d@postgresql.org
1 parent c83c025 commit d18ee6f
Copy full SHA for d18ee6f

File tree

Expand file treeCollapse file tree

12 files changed

+409
-114
lines changed
Filter options
Expand file treeCollapse file tree

12 files changed

+409
-114
lines changed

‎src/backend/commands/functioncmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/functioncmds.c
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
#include "parser/parse_func.h"
6363
#include "parser/parse_type.h"
6464
#include "pgstat.h"
65+
#include "tcop/pquery.h"
6566
#include "utils/acl.h"
6667
#include "utils/builtins.h"
6768
#include "utils/fmgroids.h"
@@ -2252,6 +2253,20 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo params, bool atomic, DestReceiver
22522253
if (fcinfo->isnull)
22532254
elog(ERROR, "procedure returned null record");
22542255

2256+
/*
2257+
* Ensure there's an active snapshot whilst we execute whatever's
2258+
* involved here. Note that this is *not* sufficient to make the
2259+
* world safe for TOAST pointers to be included in the returned data:
2260+
* the referenced data could have gone away while we didn't hold a
2261+
* snapshot. Hence, it's incumbent on PLs that can do COMMIT/ROLLBACK
2262+
* to not return TOAST pointers, unless those pointers were fetched
2263+
* after the last COMMIT/ROLLBACK in the procedure.
2264+
*
2265+
* XXX that is a really nasty, hard-to-test requirement. Is there a
2266+
* way to remove it?
2267+
*/
2268+
EnsurePortalSnapshotExists();
2269+
22552270
td = DatumGetHeapTupleHeader(retval);
22562271
tupType = HeapTupleHeaderGetTypeId(td);
22572272
tupTypmod = HeapTupleHeaderGetTypMod(td);

‎src/backend/executor/spi.c

Copy file name to clipboardExpand all lines: src/backend/executor/spi.c
+49-24Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -251,12 +251,8 @@ _SPI_commit(bool chain)
251251
/* Start the actual commit */
252252
_SPI_current->internal_xact = true;
253253

254-
/*
255-
* Before committing, pop all active snapshots to avoid error about
256-
* "snapshot %p still active".
257-
*/
258-
while (ActiveSnapshotSet())
259-
PopActiveSnapshot();
254+
/* Release snapshots associated with portals */
255+
ForgetPortalSnapshots();
260256

261257
if (chain)
262258
SaveTransactionCharacteristics();
@@ -313,6 +309,9 @@ _SPI_rollback(bool chain)
313309
/* Start the actual rollback */
314310
_SPI_current->internal_xact = true;
315311

312+
/* Release snapshots associated with portals */
313+
ForgetPortalSnapshots();
314+
316315
if (chain)
317316
SaveTransactionCharacteristics();
318317

@@ -2100,6 +2099,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
21002099
uint64 my_processed = 0;
21012100
SPITupleTable *my_tuptable = NULL;
21022101
int res = 0;
2102+
bool allow_nonatomic = plan->no_snapshots; /* legacy API name */
21032103
bool pushed_active_snap = false;
21042104
ErrorContextCallback spierrcontext;
21052105
CachedPlan *cplan = NULL;
@@ -2132,11 +2132,12 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
21322132
* In the first two cases, we can just push the snap onto the stack once
21332133
* for the whole plan list.
21342134
*
2135-
* But if the plan has no_snapshots set to true, then don't manage
2136-
* snapshots at all. The caller should then take care of that.
2135+
* Note that snapshot != InvalidSnapshot implies an atomic execution
2136+
* context.
21372137
*/
2138-
if (snapshot != InvalidSnapshot && !plan->no_snapshots)
2138+
if (snapshot != InvalidSnapshot)
21392139
{
2140+
Assert(!allow_nonatomic);
21402141
if (read_only)
21412142
{
21422143
PushActiveSnapshot(snapshot);
@@ -2211,15 +2212,39 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
22112212
stmt_list = cplan->stmt_list;
22122213

22132214
/*
2214-
* In the default non-read-only case, get a new snapshot, replacing
2215-
* any that we pushed in a previous cycle.
2215+
* If we weren't given a specific snapshot to use, and the statement
2216+
* list requires a snapshot, set that up.
22162217
*/
2217-
if (snapshot == InvalidSnapshot && !read_only && !plan->no_snapshots)
2218+
if (snapshot == InvalidSnapshot &&
2219+
(list_length(stmt_list) > 1 ||
2220+
(list_length(stmt_list) == 1 &&
2221+
PlannedStmtRequiresSnapshot(linitial_node(PlannedStmt,
2222+
stmt_list)))))
22182223
{
2219-
if (pushed_active_snap)
2220-
PopActiveSnapshot();
2221-
PushActiveSnapshot(GetTransactionSnapshot());
2222-
pushed_active_snap = true;
2224+
/*
2225+
* First, ensure there's a Portal-level snapshot. This back-fills
2226+
* the snapshot stack in case the previous operation was a COMMIT
2227+
* or ROLLBACK inside a procedure or DO block. (We can't put back
2228+
* the Portal snapshot any sooner, or we'd break cases like doing
2229+
* SET or LOCK just after COMMIT.) It's enough to check once per
2230+
* statement list, since COMMIT/ROLLBACK/CALL/DO can't appear
2231+
* within a multi-statement list.
2232+
*/
2233+
EnsurePortalSnapshotExists();
2234+
2235+
/*
2236+
* In the default non-read-only case, get a new per-statement-list
2237+
* snapshot, replacing any that we pushed in a previous cycle.
2238+
* Skip it when doing non-atomic execution, though (we rely
2239+
* entirely on the Portal snapshot in that case).
2240+
*/
2241+
if (!read_only && !allow_nonatomic)
2242+
{
2243+
if (pushed_active_snap)
2244+
PopActiveSnapshot();
2245+
PushActiveSnapshot(GetTransactionSnapshot());
2246+
pushed_active_snap = true;
2247+
}
22232248
}
22242249

22252250
foreach(lc2, stmt_list)
@@ -2231,6 +2256,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
22312256
_SPI_current->processed = 0;
22322257
_SPI_current->tuptable = NULL;
22332258

2259+
/* Check for unsupported cases. */
22342260
if (stmt->utilityStmt)
22352261
{
22362262
if (IsA(stmt->utilityStmt, CopyStmt))
@@ -2259,9 +2285,10 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
22592285

22602286
/*
22612287
* If not read-only mode, advance the command counter before each
2262-
* command and update the snapshot.
2288+
* command and update the snapshot. (But skip it if the snapshot
2289+
* isn't under our control.)
22632290
*/
2264-
if (!read_only && !plan->no_snapshots)
2291+
if (!read_only && pushed_active_snap)
22652292
{
22662293
CommandCounterIncrement();
22672294
UpdateActiveSnapshotCommandId();
@@ -2295,13 +2322,11 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
22952322
QueryCompletion qc;
22962323

22972324
/*
2298-
* If the SPI context is atomic, or we are asked to manage
2299-
* snapshots, then we are in an atomic execution context.
2300-
* Conversely, to propagate a nonatomic execution context, the
2301-
* caller must be in a nonatomic SPI context and manage
2302-
* snapshots itself.
2325+
* If the SPI context is atomic, or we were not told to allow
2326+
* nonatomic operations, tell ProcessUtility this is an atomic
2327+
* execution context.
23032328
*/
2304-
if (_SPI_current->atomic || !plan->no_snapshots)
2329+
if (_SPI_current->atomic || !allow_nonatomic)
23052330
context = PROCESS_UTILITY_QUERY;
23062331
else
23072332
context = PROCESS_UTILITY_QUERY_NONATOMIC;

‎src/backend/replication/logical/worker.c

Copy file name to clipboardExpand all lines: src/backend/replication/logical/worker.c
+27-29Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,13 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
199199
ResultRelInfo *resultRelInfo;
200200
RangeTblEntry *rte;
201201

202+
/*
203+
* Input functions may need an active snapshot, as may AFTER triggers
204+
* invoked during finish_estate. For safety, ensure an active snapshot
205+
* exists throughout all our usage of the executor.
206+
*/
207+
PushActiveSnapshot(GetTransactionSnapshot());
208+
202209
estate = CreateExecutorState();
203210

204211
rte = makeNode(RangeTblEntry);
@@ -223,6 +230,22 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
223230
return estate;
224231
}
225232

233+
/*
234+
* Finish any operations related to the executor state created by
235+
* create_estate_for_relation().
236+
*/
237+
static void
238+
finish_estate(EState *estate)
239+
{
240+
/* Handle any queued AFTER triggers. */
241+
AfterTriggerEndQuery(estate);
242+
243+
/* Cleanup. */
244+
ExecResetTupleTable(estate->es_tupleTable, false);
245+
FreeExecutorState(estate);
246+
PopActiveSnapshot();
247+
}
248+
226249
/*
227250
* Executes default values for columns for which we can't map to remote
228251
* relation columns.
@@ -634,9 +657,6 @@ apply_handle_insert(StringInfo s)
634657
RelationGetDescr(rel->localrel),
635658
&TTSOpsVirtual);
636659

637-
/* Input functions may need an active snapshot, so get one */
638-
PushActiveSnapshot(GetTransactionSnapshot());
639-
640660
/* Process and store remote tuple in the slot */
641661
oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
642662
slot_store_cstrings(remoteslot, rel, newtup.values);
@@ -651,13 +671,7 @@ apply_handle_insert(StringInfo s)
651671
apply_handle_insert_internal(estate->es_result_relation_info, estate,
652672
remoteslot);
653673

654-
PopActiveSnapshot();
655-
656-
/* Handle queued AFTER triggers. */
657-
AfterTriggerEndQuery(estate);
658-
659-
ExecResetTupleTable(estate->es_tupleTable, false);
660-
FreeExecutorState(estate);
674+
finish_estate(estate);
661675

662676
logicalrep_rel_close(rel, NoLock);
663677

@@ -778,8 +792,6 @@ apply_handle_update(StringInfo s)
778792
/* Also populate extraUpdatedCols, in case we have generated columns */
779793
fill_extraUpdatedCols(target_rte, rel->localrel);
780794

781-
PushActiveSnapshot(GetTransactionSnapshot());
782-
783795
/* Build the search tuple. */
784796
oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
785797
slot_store_cstrings(remoteslot, rel,
@@ -794,13 +806,7 @@ apply_handle_update(StringInfo s)
794806
apply_handle_update_internal(estate->es_result_relation_info, estate,
795807
remoteslot, &newtup, rel);
796808

797-
PopActiveSnapshot();
798-
799-
/* Handle queued AFTER triggers. */
800-
AfterTriggerEndQuery(estate);
801-
802-
ExecResetTupleTable(estate->es_tupleTable, false);
803-
FreeExecutorState(estate);
809+
finish_estate(estate);
804810

805811
logicalrep_rel_close(rel, NoLock);
806812

@@ -902,8 +908,6 @@ apply_handle_delete(StringInfo s)
902908
RelationGetDescr(rel->localrel),
903909
&TTSOpsVirtual);
904910

905-
PushActiveSnapshot(GetTransactionSnapshot());
906-
907911
/* Build the search tuple. */
908912
oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
909913
slot_store_cstrings(remoteslot, rel, oldtup.values);
@@ -917,13 +921,7 @@ apply_handle_delete(StringInfo s)
917921
apply_handle_delete_internal(estate->es_result_relation_info, estate,
918922
remoteslot, &rel->remoterel);
919923

920-
PopActiveSnapshot();
921-
922-
/* Handle queued AFTER triggers. */
923-
AfterTriggerEndQuery(estate);
924-
925-
ExecResetTupleTable(estate->es_tupleTable, false);
926-
FreeExecutorState(estate);
924+
finish_estate(estate);
927925

928926
logicalrep_rel_close(rel, NoLock);
929927

@@ -1248,7 +1246,7 @@ apply_handle_truncate(StringInfo s)
12481246
List *relids = NIL;
12491247
List *relids_logged = NIL;
12501248
ListCell *lc;
1251-
LOCKMODE lockmode = AccessExclusiveLock;
1249+
LOCKMODE lockmode = AccessExclusiveLock;
12521250

12531251
ensure_transaction();
12541252

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.