From 33f10586daaa13bb4e9d72ee3bf2e3a5a45eddbb Mon Sep 17 00:00:00 2001 From: Devrim Gunduz Date: Thu, 9 Jul 2026 23:18:33 +0300 Subject: [PATCH] Fix three --enable-cassert crashes in the automatic-savepoint machinery Running the regression tests against a --enable-debug --enable-cassert PostgreSQL 19 build exposes three latent bugs in the extension's save/restore bookkeeping. None of them is a PG19-specific regression; they affect all supported PostgreSQL versions and are simply newly visible because the tests are now run on an assert-enabled server. The first one is documented as a known, previously-unresolved issue in the project's own README ("Problems" section); the other two are extension-internal bookkeeping bugs. =========================================================================== 1) Assert(portal->portalContext == CurrentMemoryContext) [pquery.c] =========================================================================== pg_statement_rollback creates/releases its automatic SAVEPOINT mid-statement by directly calling DefineSavepoint()/ReleaseSavepoint() followed by CommitTransactionCommand() + CommandCounterIncrement(). Starting/committing a subtransaction this way switches CurrentMemoryContext away from the currently executing portal's own context (portal->portalContext) to CurTransactionContext, as a normal side effect of the transaction state machine. The extension already saves/restores CurrentResourceOwner around this sequence (via the MemoryContextCallback / slrPortalContext trick), but it never restored CurrentMemoryContext. On a plain build this drift was silently absorbed. On a --enable-cassert build, PortalRunMulti()'s per-statement cleanup in pquery.c hits Assert(portal->portalContext == CurrentMemoryContext), which fails and aborts the backend (SIGABRT) as soon as the automatic savepoint machinery has run at least once in the session. Fix: save CurrentMemoryContext on entry to slr_add_savepoint() and slr_release_savepoint(), and explicitly MemoryContextSwitchTo() back to it immediately after the manual CommitTransactionCommand() / CommandCounterIncrement() pair in each function, mirroring the existing CurrentResourceOwner save/restore. Both functions are fixed independently since slr_release_savepoint() can be called on its own (e.g. just before a "SET TRANSACTION ..." statement) without a following slr_add_savepoint() call to mask the drift. =========================================================================== 2) Assert(oldresowner == NULL) after a read-only statement [pg_statement_rollback.c, slr_save_resowner()] =========================================================================== slr_ExecutorStart() calls slr_save_resowner() for every top-level statement, stashing CurrentResourceOwner and PortalContext into the static oldresowner/slrPortalContext variables. But slr_ExecutorEnd() only consumes them (through slr_add_savepoint()) for statements that actually get an automatic savepoint -- with the default pg_statement_rollback.enable_writeonly = on, write statements only. A read-only statement (plain SELECT) therefore orphans the saved values. The next call to slr_save_resowner() -- from the next DML statement, or from ProcessUtility for a client SAVEPOINT command -- finds oldresowner already set and trips Assert(oldresowner == NULL), aborting the backend. This means even the trivial sequence BEGIN; SELECT 1; INSERT ...; crashes an assert-enabled server. On production builds the stale value was silently (and harmlessly) overwritten by the next save, which is why this went unnoticed; the stale slrPortalContext however points at a portal context that is destroyed once the read-only statement's portal is dropped, which is a genuine dangling pointer on any build. Fix: in slr_ExecutorEnd(), when handling a top-level statement that did not take the release/add-savepoint branch, explicitly discard oldresowner and slrPortalContext. slr_save_resowner() only reads CurrentResourceOwner/PortalContext, so discarding is sufficient; and keeping the discard explicit preserves the assertion's value for catching genuine double-saves within a single statement. =========================================================================== 3) Assert(oldresowner == NULL) after an execution-time error [pg_statement_rollback.c, slr_save_resowner()] =========================================================================== If a statement passes parse analysis and planning but fails during execution (e.g. a runtime data or constraint error), ExecutorEnd is never reached -- the error unwinds straight through PG_CATCH -- so the oldresowner/slrPortalContext values saved at ExecutorStart are left stale, referring to (sub)transaction state that the error's abort processing tears down. The next slr_save_resowner() call then trips the same Assert; on production builds the leftover slrPortalContext could let a later slr_add_savepoint() register a callback on freed memory. Fix: in disable_differed_slr(), the extension's emit_log_hook callback that already runs on every ERROR, also reset oldresowner and slrPortalContext to NULL. (Note: errors raised at parse-analysis time, such as failed literal coercion, never reach the executor hooks and leave nothing stale; this fix covers execution-time failures, while fix 2 covers the read-only path.) =========================================================================== 4) Assert(context != CurrentMemoryContext) in mcxt.c during ROLLBACK TO SAVEPOINT after an error [PostgreSQL 18+ only] =========================================================================== PostgreSQL 18 introduced TransactionState->priorContext: AtSubStart_Memory() snapshots CurrentMemoryContext when a subtransaction starts, and AtSubCleanup_Memory() switches back to it when the subtransaction is cleaned up after an abort (an error followed by ROLLBACK TO SAVEPOINT). On PostgreSQL 17 and older the cleanup path switched to s->parent->curTransactionContext instead. The extension creates its automatic savepoint's subtransaction from inside statement execution, at a point where a per-portal memory context is current (the executing statement's portalContext). That portal -- and its memory context -- is destroyed at the end of that statement, long before the automatic savepoint's subtransaction ends. priorContext therefore ends up as a dangling pointer to a freed context node. If the subtransaction is subsequently aborted by an error and cleaned up by ROLLBACK TO, AtSubCleanup_Memory() switches CurrentMemoryContext to the freed portal context: undefined behavior on production builds, and on --enable-cassert builds it surfaces as Assert(context != CurrentMemoryContext) failing in MemoryContextDeleteOnly() (mcxt.c) when the recycled pointer collides with a context node being deleted during the cleanup. Observed with test/sql/04_slr_log_writeonly.sql: the savepoint is recreated during "SELECT test_insert()" (deferred release+add for a nested write), the following UPDATE fails at parse analysis, and the client's ROLLBACK TO SAVEPOINT aze crashes in CleanupSubTransaction. Note the commit path (AtSubCommit_Memory) never uses priorContext, which is why successful savepoint release/recreate cycles are unaffected and only the error + ROLLBACK TO path crashes. Fix: in slr_add_savepoint(), switch to the (parent's) transaction-lifetime CurTransactionContext before calling DefineSavepoint()/CommitTransactionCommand(), so that the subtransaction's priorContext refers to a context guaranteed to outlive it -- semantically identical to what AtSubCleanup_Memory() used on PostgreSQL 17 and older. The portal context is restored afterwards as before (fix 1), keeping the pquery.c assertion happy. This also hardens the BEGIN-time savepoint path, which had the same latent hazard on PostgreSQL 18+ even before these patches (there the subtransaction was created with the BEGIN statement's portal context current). No behavior change on non-assert builds for fix (1); fixes (2) and (3) also close latent dangling-pointer bugs independent of assert builds. Hacked by Claude, tested by me. --- README.md | 57 -------------------- pg_statement_rollback.c | 112 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index c40a588..87c9aab 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,6 @@ * [Configuration](#configuration) * [Use of the extension](#use-of-the-extension) * [Performances](#performances) -* [Problems](#problems) * [Authors](#authors) * [License](#license) @@ -234,62 +233,6 @@ of limiting savepoint to write statements only. A special script should be built with more than 64 (PGPROC_MAX_CACHED_SUBXIDS) statements in a transaction to start playing with bottlenecks. -### [Problems](#problems) - -When compiled with assert enabled (`--enable-cassert`) PostgreSQL will crash -when the extension is used. At line 1327 of ./src/backend/tcop/pquery.c the -following assert fail: - -``` -/* - * Clear subsidiary contexts to recover temporary memory. - */ -Assert(portal->portalContext == CurrentMemoryContext); -``` - -Actually with the extension the memory context is not CurrentMemoryContext -as expected. - -``` -(gdb) b pquery.c:1327 -Breakpoint 1 at 0x55792fd7a04d: file pquery.c, line 1327. -(gdb) c -Continuing. - -Breakpoint 1, PortalRunMulti (portal=portal@entry=0x5579316e3e10, isTopLevel=isTopLevel@entry=true, - setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x557931755ce8, altdest=altdest@entry=0x557931755ce8, - qc=qc@entry=0x7ffc4aa1f8a0) at pquery.c:1327 -1327 Assert(portal->portalContext == CurrentMemoryContext); -(gdb) p portal->sourceText -$1 = 0x557931679c80 "INSERT INTO savepoint_test SELECT 1;" -(gdb) p MemoryContextStats(portal->portalContext) -$2 = void -(gdb) -``` -The memory context dump output -``` -PortalContext: 1024 total in 1 blocks; 704 free (1 chunks); 320 used: -Grand total: 1024 bytes in 1 blocks; 704 free (1 chunks); 320 used -``` - -Clearly the assert in pquery.c doesn't allow our particular use for server side -statement-level rollback, PostgreSQL code should probably be modified because -we don't have possibilities to fix that at the extension level. - -Here how to reproduce the crash: - -``` -SELECT pg_backend_pid(); -LOAD 'pg_statement_rollback.so'; -SET pg_statement_rollback.enabled = 1; -SET client_min_messages TO LOG; -SET log_statement TO 'all'; -BEGIN; -CREATE TABLE savepoint_test(id integer); --- run gdb on pid displayed above then => b pquery.c:1327 -INSERT INTO savepoint_test SELECT 1; -- crash -``` - ### [Authors](#authors) - Julien Rouhaud diff --git a/pg_statement_rollback.c b/pg_statement_rollback.c index ddfb6fa..5547e67 100644 --- a/pg_statement_rollback.c +++ b/pg_statement_rollback.c @@ -720,6 +720,35 @@ slr_ExecutorEnd(QueryDesc *queryDesc) slr_defered_save_resowner = false; } + else if ( +#if PG_VERSION_NUM >= 90500 + !IN_PARALLEL_WORKER && +#endif + slr_nest_executor_level == 0 && oldresowner != NULL) + { + /* + * slr_ExecutorStart() saved the resource owner and portal context + * for every top-level statement, but the branch above only consumes + * them (through slr_add_savepoint()) for statements that actually + * get an automatic savepoint -- with the default + * pg_statement_rollback.enable_writeonly = on, that means write + * statements only. For a read-only statement (plain SELECT) the + * saved values would otherwise be orphaned here and the next call + * to slr_save_resowner() -- e.g. from the next DML, or from + * ProcessUtility for a client SAVEPOINT -- would trip + * Assert(oldresowner == NULL) on a --enable-cassert build and + * abort the backend. A stale slrPortalContext is also dangerous + * on any build, since it points at this statement's portal + * context, which is destroyed once the portal is dropped. + * + * Nothing was modified: slr_save_resowner() only reads + * CurrentResourceOwner and PortalContext, so discarding the + * stashed values is all that is needed. + */ + elog(DEBUG1, "RSL: ExecutorEnd discarding unused saved resource owner (read-only statement)."); + oldresowner = NULL; + slrPortalContext = NULL; + } if (prev_ExecutorEnd) prev_ExecutorEnd(queryDesc); @@ -781,9 +810,35 @@ slr_add_savepoint(void) if (slr_enabled && slr_xact_opened) { MemoryContextCallback *slr_cb = NULL; + MemoryContext oldcontext = CurrentMemoryContext; elog(DEBUG1, "RSL: adding savepoint %s.", slr_savepoint_name); + /* + * PostgreSQL 18+ snapshots CurrentMemoryContext into the new + * subtransaction's priorContext at AtSubStart_Memory() time, and + * AtSubCleanup_Memory() switches back to it when the + * subtransaction is cleaned up after an abort (error followed by + * ROLLBACK TO). We are called with a per-portal memory context + * current, and that portal (and its context) dies at the end of + * the current statement -- long before our automatic savepoint's + * subtransaction does. If the subtransaction is later aborted + * and cleaned up, core would switch CurrentMemoryContext to the + * freed portal context: undefined behavior on any build, and on + * --enable-cassert builds it manifests as + * Assert(context != CurrentMemoryContext) failing in mcxt.c when + * the recycled pointer collides with a context being deleted. + * + * Create the subtransaction with the (parent's) transaction- + * lifetime CurTransactionContext current instead, so that + * priorContext refers to a context that is guaranteed to outlive + * the subtransaction. This matches what AtSubCleanup_Memory() + * switched to implicitly on PostgreSQL 17 and older + * (s->parent->curTransactionContext). The portal context is + * restored below, after the savepoint machinery has run. + */ + MemoryContextSwitchTo(CurTransactionContext); + /* Define savepoint */ DefineSavepoint(slr_savepoint_name); elog(DEBUG1, "RSL: CommitTransactionCommand."); @@ -791,6 +846,19 @@ slr_add_savepoint(void) elog(DEBUG1, "RSL: CommandCounterIncrement."); CommandCounterIncrement(); + /* + * DefineSavepoint()/CommitTransactionCommand() start and commit a + * subtransaction, which as a side effect switches + * CurrentMemoryContext away from the portal's own context (to + * CurTransactionContext). We must switch it back before returning, + * otherwise PortalRunMulti()'s cleanup will later find + * portal->portalContext != CurrentMemoryContext and, on a + * --enable-cassert build, trip + * Assert(portal->portalContext == CurrentMemoryContext) in + * pquery.c, crashing the backend. + */ + MemoryContextSwitchTo(oldcontext); + /* * Backup the new resowner, will be restore the end of execution on the * Portal memory context callback @@ -832,6 +900,7 @@ slr_release_savepoint(void) if (slr_enabled && slr_xact_opened && slr_pending) { + MemoryContext oldcontext = CurrentMemoryContext; #if PG_VERSION_NUM < 110000 List *options = NIL; DefElem *elem = NULL; @@ -854,6 +923,17 @@ slr_release_savepoint(void) CommitTransactionCommand(); CommandCounterIncrement(); + /* + * As in slr_add_savepoint(), ReleaseSavepoint()/CommitTransactionCommand() + * leaves CurrentMemoryContext pointing at CurTransactionContext + * instead of the portal's own context. Restore it here too: this + * function can be called on its own (e.g. right before "SET + * TRANSACTION ...") without a following slr_add_savepoint() call to + * paper over the drift, so it must not leave CurrentMemoryContext + * corrupted itself. + */ + MemoryContextSwitchTo(oldcontext); + slr_pending = false; /* Manually log the order if needed */ @@ -938,8 +1018,40 @@ disable_differed_slr(ErrorData *edata) { /* Do not ask for automatic savepoint if previous statement has an error */ if (edata->elevel >= ERROR) + { slr_defered_save_resowner = false; + /* + * If slr_save_resowner() ran for the statement that is now + * erroring out (in ExecutorStart, or in ProcessUtility before + * executing the utility command), the matching slr_add_savepoint() + * that would normally consume and clear oldresowner/slrPortalContext + * never gets to run: the executor/utility call never reaches its + * normal completion path on error, it just unwinds via PG_CATCH. + * + * The (sub)transaction state those variables refer to is about to + * be torn down by the error abort machinery, so they must not + * survive past this point: + * + * - oldresowner: if left set, the next slr_save_resowner() call + * (for the next statement) trips + * Assert(oldresowner == NULL) on a --enable-cassert build, and + * crashes the backend. + * + * - slrPortalContext: if left set, it may point at a portal + * context already destroyed by the abort, and a later + * slr_add_savepoint() would allocate into it / register a reset + * callback on freed memory. + * + * CurrentResourceOwner itself was never modified by + * slr_save_resowner() (it only reads it), so there is nothing to + * restore here -- simply forgetting the stashed values is + * sufficient and correct. + */ + oldresowner = NULL; + slrPortalContext = NULL; + } + /* Continue chain to previous hook */ if (prev_log_hook) (*prev_log_hook) (edata);