Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devrimgunduz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 0 additions & 57 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* [Configuration](#configuration)
* [Use of the extension](#use-of-the-extension)
* [Performances](#performances)
* [Problems](#problems)
* [Authors](#authors)
* [License](#license)

Expand Down Expand Up @@ -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: <unnamed>
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
Expand Down
112 changes: 112 additions & 0 deletions pg_statement_rollback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -781,16 +810,55 @@ 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.");
CommitTransactionCommand();
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
Expand Down Expand Up @@ -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;
Expand All @@ -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 */
Expand Down Expand Up @@ -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);
Expand Down