From 12d768e70497afc5a57acf73c251316997b5175a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 28 Feb 2022 12:54:12 -0500 Subject: [PATCH] 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 --- src/backend/access/transam/xact.c | 32 ++++++++++++++----------------- src/backend/executor/spi.c | 16 ++++++++-------- src/include/access/xact.h | 12 ++++++++++-- 3 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index bb1f106946..adf763a8ea 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2983,24 +2983,20 @@ StartTransactionCommand(void) * GUC system resets the characteristics at transaction end, so for example * just skipping the reset in StartTransaction() won't work.) */ -static int save_XactIsoLevel; -static bool save_XactReadOnly; -static bool save_XactDeferrable; - void -SaveTransactionCharacteristics(void) +SaveTransactionCharacteristics(SavedTransactionCharacteristics *s) { - save_XactIsoLevel = XactIsoLevel; - save_XactReadOnly = XactReadOnly; - save_XactDeferrable = XactDeferrable; + s->save_XactIsoLevel = XactIsoLevel; + s->save_XactReadOnly = XactReadOnly; + s->save_XactDeferrable = XactDeferrable; } void -RestoreTransactionCharacteristics(void) +RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s) { - XactIsoLevel = save_XactIsoLevel; - XactReadOnly = save_XactReadOnly; - XactDeferrable = save_XactDeferrable; + XactIsoLevel = s->save_XactIsoLevel; + XactReadOnly = s->save_XactReadOnly; + XactDeferrable = s->save_XactDeferrable; } @@ -3011,9 +3007,9 @@ void CommitTransactionCommand(void) { TransactionState s = CurrentTransactionState; + SavedTransactionCharacteristics savetc; - if (s->chain) - SaveTransactionCharacteristics(); + SaveTransactionCharacteristics(&savetc); switch (s->blockState) { @@ -3071,7 +3067,7 @@ CommitTransactionCommand(void) StartTransaction(); s->blockState = TBLOCK_INPROGRESS; s->chain = false; - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); } break; @@ -3097,7 +3093,7 @@ CommitTransactionCommand(void) StartTransaction(); s->blockState = TBLOCK_INPROGRESS; s->chain = false; - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); } break; @@ -3115,7 +3111,7 @@ CommitTransactionCommand(void) StartTransaction(); s->blockState = TBLOCK_INPROGRESS; s->chain = false; - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); } break; @@ -3182,7 +3178,7 @@ CommitTransactionCommand(void) StartTransaction(); s->blockState = TBLOCK_INPROGRESS; s->chain = false; - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); } } else if (s->blockState == TBLOCK_PREPARE) diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 7971050746..5b353cb93a 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -228,6 +228,7 @@ static void _SPI_commit(bool chain) { MemoryContext oldcontext = CurrentMemoryContext; + SavedTransactionCharacteristics savetc; /* * 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), errmsg("cannot commit while a subtransaction is active"))); - /* XXX this ain't re-entrant enough for my taste */ if (chain) - SaveTransactionCharacteristics(); + SaveTransactionCharacteristics(&savetc); /* Catch any error occurring during the COMMIT */ PG_TRY(); @@ -281,7 +281,7 @@ _SPI_commit(bool chain) /* Immediately start a new transaction */ StartTransactionCommand(); if (chain) - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); MemoryContextSwitchTo(oldcontext); @@ -305,7 +305,7 @@ _SPI_commit(bool chain) /* ... and start a new one */ StartTransactionCommand(); if (chain) - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); MemoryContextSwitchTo(oldcontext); @@ -333,6 +333,7 @@ static void _SPI_rollback(bool chain) { MemoryContext oldcontext = CurrentMemoryContext; + SavedTransactionCharacteristics savetc; /* see under SPI_commit() */ if (_SPI_current->atomic) @@ -346,9 +347,8 @@ _SPI_rollback(bool chain) (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION), errmsg("cannot roll back while a subtransaction is active"))); - /* XXX this ain't re-entrant enough for my taste */ if (chain) - SaveTransactionCharacteristics(); + SaveTransactionCharacteristics(&savetc); /* Catch any error occurring during the ROLLBACK */ PG_TRY(); @@ -373,7 +373,7 @@ _SPI_rollback(bool chain) /* Immediately start a new transaction */ StartTransactionCommand(); if (chain) - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); MemoryContextSwitchTo(oldcontext); @@ -398,7 +398,7 @@ _SPI_rollback(bool chain) /* ... and start a new one */ StartTransactionCommand(); if (chain) - RestoreTransactionCharacteristics(); + RestoreTransactionCharacteristics(&savetc); MemoryContextSwitchTo(oldcontext); diff --git a/src/include/access/xact.h b/src/include/access/xact.h index 17a6fa4abd..062cc7e17d 100644 --- a/src/include/access/xact.h +++ b/src/include/access/xact.h @@ -135,6 +135,14 @@ typedef enum typedef void (*SubXactCallback) (SubXactEvent event, SubTransactionId mySubid, 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 @@ -399,8 +407,8 @@ extern bool TransactionIdIsCurrentTransactionId(TransactionId xid); extern void CommandCounterIncrement(void); extern void ForceSyncCommit(void); extern void StartTransactionCommand(void); -extern void SaveTransactionCharacteristics(void); -extern void RestoreTransactionCharacteristics(void); +extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s); +extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s); extern void CommitTransactionCommand(void); extern void AbortCurrentTransaction(void); extern void BeginTransactionBlock(void);