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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index bb1f1069463119ade9290c4dae12da7b0467ea47..adf763a8ea4415c810d0cae059c13394aa4d1ac6 100644 (file)
--- 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 7971050746feda11afeff6b7909b05ec433a8dc4..5b353cb93a7652ce29e5cbc2003d3356a11224e7 100644 (file)
--- 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 17a6fa4abdbb764a2dcc67847cb6ebaba14901f1..062cc7e17d8a7de95badddaa6a85f6fa7f43a262 100644 (file)
--- 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);