Don't use static storage for SaveTransactionCharacteristics().

This is pretty queasy-making on general principles, and the more so
once you notice that CommitTransactionCommand() is actually stomping
on the values saved by _SPI_commit().  It's okay as long as the
active values didn't change during HoldPinnedPortals(); but that's
a larger assumption than I think we want to make, especially since
the fix is so simple.

Discussion: https://postgr.es/m/1533956.1645731245@sss.pgh.pa.us
This commit is contained in:
Tom Lane 2022-02-28 12:54:12 -05:00
parent 2e517818f4
commit 12d768e704
3 changed files with 32 additions and 28 deletions

View File

@ -2983,24 +2983,20 @@ StartTransactionCommand(void)
* GUC system resets the characteristics at transaction end, so for example * GUC system resets the characteristics at transaction end, so for example
* just skipping the reset in StartTransaction() won't work.) * just skipping the reset in StartTransaction() won't work.)
*/ */
static int save_XactIsoLevel;
static bool save_XactReadOnly;
static bool save_XactDeferrable;
void void
SaveTransactionCharacteristics(void) SaveTransactionCharacteristics(SavedTransactionCharacteristics *s)
{ {
save_XactIsoLevel = XactIsoLevel; s->save_XactIsoLevel = XactIsoLevel;
save_XactReadOnly = XactReadOnly; s->save_XactReadOnly = XactReadOnly;
save_XactDeferrable = XactDeferrable; s->save_XactDeferrable = XactDeferrable;
} }
void void
RestoreTransactionCharacteristics(void) RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
{ {
XactIsoLevel = save_XactIsoLevel; XactIsoLevel = s->save_XactIsoLevel;
XactReadOnly = save_XactReadOnly; XactReadOnly = s->save_XactReadOnly;
XactDeferrable = save_XactDeferrable; XactDeferrable = s->save_XactDeferrable;
} }
@ -3011,9 +3007,9 @@ void
CommitTransactionCommand(void) CommitTransactionCommand(void)
{ {
TransactionState s = CurrentTransactionState; TransactionState s = CurrentTransactionState;
SavedTransactionCharacteristics savetc;
if (s->chain) SaveTransactionCharacteristics(&savetc);
SaveTransactionCharacteristics();
switch (s->blockState) switch (s->blockState)
{ {
@ -3071,7 +3067,7 @@ CommitTransactionCommand(void)
StartTransaction(); StartTransaction();
s->blockState = TBLOCK_INPROGRESS; s->blockState = TBLOCK_INPROGRESS;
s->chain = false; s->chain = false;
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
} }
break; break;
@ -3097,7 +3093,7 @@ CommitTransactionCommand(void)
StartTransaction(); StartTransaction();
s->blockState = TBLOCK_INPROGRESS; s->blockState = TBLOCK_INPROGRESS;
s->chain = false; s->chain = false;
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
} }
break; break;
@ -3115,7 +3111,7 @@ CommitTransactionCommand(void)
StartTransaction(); StartTransaction();
s->blockState = TBLOCK_INPROGRESS; s->blockState = TBLOCK_INPROGRESS;
s->chain = false; s->chain = false;
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
} }
break; break;
@ -3182,7 +3178,7 @@ CommitTransactionCommand(void)
StartTransaction(); StartTransaction();
s->blockState = TBLOCK_INPROGRESS; s->blockState = TBLOCK_INPROGRESS;
s->chain = false; s->chain = false;
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
} }
} }
else if (s->blockState == TBLOCK_PREPARE) else if (s->blockState == TBLOCK_PREPARE)

View File

@ -228,6 +228,7 @@ static void
_SPI_commit(bool chain) _SPI_commit(bool chain)
{ {
MemoryContext oldcontext = CurrentMemoryContext; MemoryContext oldcontext = CurrentMemoryContext;
SavedTransactionCharacteristics savetc;
/* /*
* Complain if we are in a context that doesn't permit transaction * Complain if we are in a context that doesn't permit transaction
@ -255,9 +256,8 @@ _SPI_commit(bool chain)
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION), (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
errmsg("cannot commit while a subtransaction is active"))); errmsg("cannot commit while a subtransaction is active")));
/* XXX this ain't re-entrant enough for my taste */
if (chain) if (chain)
SaveTransactionCharacteristics(); SaveTransactionCharacteristics(&savetc);
/* Catch any error occurring during the COMMIT */ /* Catch any error occurring during the COMMIT */
PG_TRY(); PG_TRY();
@ -281,7 +281,7 @@ _SPI_commit(bool chain)
/* Immediately start a new transaction */ /* Immediately start a new transaction */
StartTransactionCommand(); StartTransactionCommand();
if (chain) if (chain)
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);
@ -305,7 +305,7 @@ _SPI_commit(bool chain)
/* ... and start a new one */ /* ... and start a new one */
StartTransactionCommand(); StartTransactionCommand();
if (chain) if (chain)
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);
@ -333,6 +333,7 @@ static void
_SPI_rollback(bool chain) _SPI_rollback(bool chain)
{ {
MemoryContext oldcontext = CurrentMemoryContext; MemoryContext oldcontext = CurrentMemoryContext;
SavedTransactionCharacteristics savetc;
/* see under SPI_commit() */ /* see under SPI_commit() */
if (_SPI_current->atomic) if (_SPI_current->atomic)
@ -346,9 +347,8 @@ _SPI_rollback(bool chain)
(errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION), (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
errmsg("cannot roll back while a subtransaction is active"))); errmsg("cannot roll back while a subtransaction is active")));
/* XXX this ain't re-entrant enough for my taste */
if (chain) if (chain)
SaveTransactionCharacteristics(); SaveTransactionCharacteristics(&savetc);
/* Catch any error occurring during the ROLLBACK */ /* Catch any error occurring during the ROLLBACK */
PG_TRY(); PG_TRY();
@ -373,7 +373,7 @@ _SPI_rollback(bool chain)
/* Immediately start a new transaction */ /* Immediately start a new transaction */
StartTransactionCommand(); StartTransactionCommand();
if (chain) if (chain)
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);
@ -398,7 +398,7 @@ _SPI_rollback(bool chain)
/* ... and start a new one */ /* ... and start a new one */
StartTransactionCommand(); StartTransactionCommand();
if (chain) if (chain)
RestoreTransactionCharacteristics(); RestoreTransactionCharacteristics(&savetc);
MemoryContextSwitchTo(oldcontext); MemoryContextSwitchTo(oldcontext);

View File

@ -135,6 +135,14 @@ typedef enum
typedef void (*SubXactCallback) (SubXactEvent event, SubTransactionId mySubid, typedef void (*SubXactCallback) (SubXactEvent event, SubTransactionId mySubid,
SubTransactionId parentSubid, void *arg); SubTransactionId parentSubid, void *arg);
/* Data structure for Save/RestoreTransactionCharacteristics */
typedef struct SavedTransactionCharacteristics
{
int save_XactIsoLevel;
bool save_XactReadOnly;
bool save_XactDeferrable;
} SavedTransactionCharacteristics;
/* ---------------- /* ----------------
* transaction-related XLOG entries * transaction-related XLOG entries
@ -399,8 +407,8 @@ extern bool TransactionIdIsCurrentTransactionId(TransactionId xid);
extern void CommandCounterIncrement(void); extern void CommandCounterIncrement(void);
extern void ForceSyncCommit(void); extern void ForceSyncCommit(void);
extern void StartTransactionCommand(void); extern void StartTransactionCommand(void);
extern void SaveTransactionCharacteristics(void); extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s);
extern void RestoreTransactionCharacteristics(void); extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s);
extern void CommitTransactionCommand(void); extern void CommitTransactionCommand(void);
extern void AbortCurrentTransaction(void); extern void AbortCurrentTransaction(void);
extern void BeginTransactionBlock(void); extern void BeginTransactionBlock(void);