Fix three --enable-cassert crashes in the automatic-savepoint machinery

[devrimgunduz]

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.

PG 18 compatibility issue

PG18 introduced TransactionState->priorContext: when a subtransaction starts, core snapshots CurrentMemoryContext, and the abort-cleanup path (AtSubCleanup_Memory, run by ROLLBACK TO after an error) switches back to it before deleting the subtransaction's contexts. On PG17 and older, cleanup switched to s->parent->curTransactionContext — always transaction-lifetime, always safe.

The extension creates its automatic savepoint mid-statement, when a portal context is current. That portal dies at the end of the statement; the subtransaction lives on. So priorContext dangles, and the first error + ROLLBACK TO aze makes core switch CurrentMemoryContext to freed memory. In 04_slr_log_writeonly.sql the malloc recycling happened to line up so the cassert assert caught it; on a production build this is silent use-after-free. My fix #1 made the ExecutorEnd path capture the portal context (previously it happened to leave TopTransactionContext current there), but the BEGIN-time savepoint path had this hazard on PG18+ even in pristine upstream code — so this was coming for the extension on PG18/19 regardless.

The fix is one line plus commentary: switch to CurTransactionContext before DefineSavepoint()/CommitTransactionCommand() in slr_add_savepoint(), so priorContext captures the parent's transaction-lifetime context — byte-for-byte the same context PG≤17's cleanup used — then restore the portal context afterwards so the pquery.c assert (fix 1 ) stays satisfied. Also worth noting the diagnostic asymmetry it explains: the commit path (AtSubCommit_Memory) never touches priorContext, which is why hundreds of successful savepoint release/recreate cycles across tests 01–03 ran clean and only the error + ROLLBACK TO combination detonated.

Hacked by Claude, tested by me, so please review carefully.