Fix Portal snapshot tracking to handle subtransactions properly.

Commit 84f5c2908 forgot to consider the possibility that
EnsurePortalSnapshotExists could run inside a subtransaction with
lifespan shorter than the Portal's.  In that case, the new active
snapshot would be popped at the end of the subtransaction, leaving
a dangling pointer in the Portal, with mayhem ensuing.

To fix, make sure the ActiveSnapshot stack entry is marked with
the same subtransaction nesting level as the associated Portal.
It's certainly safe to do so since we won't be here at all unless
the stack is empty; hence we can't create an out-of-order stack.

Let's also apply this logic in the case where PortalRunUtility
sets portalSnapshot, just to be sure that path can't cause similar
problems.  It's slightly less clear that that path can't create
an out-of-order stack, so add an assertion guarding it.

Report and patch by Bertrand Drouvot (with kibitzing by me).
Back-patch to v11, like the previous commit.

Discussion: https://postgr.es/m/ff82b8c5-77f4-3fe7-6028-fcf3303e82dd@amazon.com

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 2f0c4b1e137dcbcfb3a86433a181a9d5b85c606d..8815127781809dcc695b705c00a16c0700bb40a2 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4649,6 +4649,7 @@ CommitSubTransaction(void)
    AfterTriggerEndSubXact(true);
    AtSubCommit_Portals(s->subTransactionId,
                        s->parent->subTransactionId,
+                       s->parent->nestingLevel,
                        s->parent->curTransactionOwner);
    AtEOSubXact_LargeObject(true, s->subTransactionId,
                            s->parent->subTransactionId);

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index a03408886845ca5b2368504432c79f1ef31141d5..0dfead08619b77443596a916064cf84ad83dbfd9 100644 (file)
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -495,7 +495,9 @@ PortalStart(Portal portal, ParamListInfo params,
                 * We could remember the snapshot in portal->portalSnapshot,
                 * but presently there seems no need to, as this code path
                 * cannot be used for non-atomic execution.  Hence there can't
-                * be any commit/abort that might destroy the snapshot.
+                * be any commit/abort that might destroy the snapshot.  Since
+                * we don't do that, there's also no need to force a
+                * non-default nesting level for the snapshot.
                 */
 
                /*
@@ -1155,9 +1157,15 @@ PortalRunUtility(Portal portal, PlannedStmt *pstmt,
            snapshot = RegisterSnapshot(snapshot);
            portal->holdSnapshot = snapshot;
        }
-       /* In any case, make the snapshot active and remember it in portal */
-       PushActiveSnapshot(snapshot);
-       /* PushActiveSnapshot might have copied the snapshot */
+
+       /*
+        * In any case, make the snapshot active and remember it in portal.
+        * Because the portal now references the snapshot, we must tell
+        * snapmgr.c that the snapshot belongs to the portal's transaction
+        * level, else we risk portalSnapshot becoming a dangling pointer.
+        */
+       PushActiveSnapshotWithLevel(snapshot, portal->createLevel);
+       /* PushActiveSnapshotWithLevel might have copied the snapshot */
        portal->portalSnapshot = GetActiveSnapshot();
    }
    else
@@ -1813,8 +1821,13 @@ EnsurePortalSnapshotExists(void)
        elog(ERROR, "cannot execute SQL without an outer snapshot or portal");
    Assert(portal->portalSnapshot == NULL);
 
-   /* Create a new snapshot and make it active */
-   PushActiveSnapshot(GetTransactionSnapshot());
-   /* PushActiveSnapshot might have copied the snapshot */
+   /*
+    * Create a new snapshot, make it active, and remember it in portal.
+    * Because the portal now references the snapshot, we must tell snapmgr.c
+    * that the snapshot belongs to the portal's transaction level, else we
+    * risk portalSnapshot becoming a dangling pointer.
+    */
+   PushActiveSnapshotWithLevel(GetTransactionSnapshot(), portal->createLevel);
+   /* PushActiveSnapshotWithLevel might have copied the snapshot */
    portal->portalSnapshot = GetActiveSnapshot();
 }

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 6ac0b2666af0aeeb35d8915f26b4614031158b74..e7a45423532a07a7422fadf5cb16e69667db6f2b 100644 (file)
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -210,6 +210,7 @@ CreatePortal(const char *name, bool allowDup, bool dupSilent)
    portal->cleanup = PortalCleanup;
    portal->createSubid = GetCurrentSubTransactionId();
    portal->activeSubid = portal->createSubid;
+   portal->createLevel = GetCurrentTransactionNestLevel();
    portal->strategy = PORTAL_MULTI_QUERY;
    portal->cursorOptions = CURSOR_OPT_NO_SCROLL;
    portal->atStart = true;
@@ -655,6 +656,7 @@ HoldPortal(Portal portal)
     */
    portal->createSubid = InvalidSubTransactionId;
    portal->activeSubid = InvalidSubTransactionId;
+   portal->createLevel = 0;
 }
 
 /*
@@ -938,6 +940,7 @@ PortalErrorCleanup(void)
 void
 AtSubCommit_Portals(SubTransactionId mySubid,
                    SubTransactionId parentSubid,
+                   int parentLevel,
                    ResourceOwner parentXactOwner)
 {
    HASH_SEQ_STATUS status;
@@ -952,6 +955,7 @@ AtSubCommit_Portals(SubTransactionId mySubid,
        if (portal->createSubid == mySubid)
        {
            portal->createSubid = parentSubid;
+           portal->createLevel = parentLevel;
            if (portal->resowner)
                ResourceOwnerNewParent(portal->resowner, parentXactOwner);
        }

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index edf59efc29d8f867e3a15ef9bd8f3ab5b7783d5a..f4401060643bb91b879f845493679d01cdf4f299 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -731,10 +731,25 @@ FreeSnapshot(Snapshot snapshot)
  */
 void
 PushActiveSnapshot(Snapshot snap)
+{
+   PushActiveSnapshotWithLevel(snap, GetCurrentTransactionNestLevel());
+}
+
+/*
+ * PushActiveSnapshotWithLevel
+ *     Set the given snapshot as the current active snapshot
+ *
+ * Same as PushActiveSnapshot except that caller can specify the
+ * transaction nesting level that "owns" the snapshot.  This level
+ * must not be deeper than the current top of the snapshot stack.
+ */
+void
+PushActiveSnapshotWithLevel(Snapshot snap, int snap_level)
 {
    ActiveSnapshotElt *newactive;
 
    Assert(snap != InvalidSnapshot);
+   Assert(ActiveSnapshot == NULL || snap_level >= ActiveSnapshot->as_level);
 
    newactive = MemoryContextAlloc(TopTransactionContext, sizeof(ActiveSnapshotElt));
 
@@ -748,7 +763,7 @@ PushActiveSnapshot(Snapshot snap)
        newactive->as_snap = snap;
 
    newactive->as_next = ActiveSnapshot;
-   newactive->as_level = GetCurrentTransactionNestLevel();
+   newactive->as_level = snap_level;
 
    newactive->as_snap->active_count++;
 

diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index a3564d12aa9a4f4cce984353c1dd9718257329d7..6ec99d3b7dbe8a284a4fbd866baf033d3884fbf0 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -193,6 +193,8 @@ typedef struct PortalData
    TimestampTz creation_time;  /* time at which this portal was defined */
    bool        visible;        /* include this portal in pg_cursors? */
 
+   /* Stuff added at the end to avoid ABI break in stable branches: */
+
    /*
     * Outermost ActiveSnapshot for execution of the portal's queries.  For
     * all but a few utility commands, we require such a snapshot to exist.
@@ -200,6 +202,7 @@ typedef struct PortalData
     * and helps to reduce thrashing of the process's exposed xmin.
     */
    Snapshot    portalSnapshot; /* active snapshot, or NULL if none */
+   int         createLevel;    /* creating subxact's nesting level */
 }          PortalData;
 
 /*
@@ -217,6 +220,7 @@ extern void AtCleanup_Portals(void);
 extern void PortalErrorCleanup(void);
 extern void AtSubCommit_Portals(SubTransactionId mySubid,
                    SubTransactionId parentSubid,
+                   int parentLevel,
                    ResourceOwner parentXactOwner);
 extern void AtSubAbort_Portals(SubTransactionId mySubid,
                   SubTransactionId parentSubid,

diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index dacc9e38efbb9e8d79a060403cd6ba1e98560e54..9280832de4d296260b6a8d015de8f17721f897de 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -71,6 +71,7 @@ extern void InvalidateCatalogSnapshot(void);
 extern void InvalidateCatalogSnapshotConditionally(void);
 
 extern void PushActiveSnapshot(Snapshot snapshot);
+extern void PushActiveSnapshotWithLevel(Snapshot snapshot, int snap_level);
 extern void PushCopiedSnapshot(Snapshot snapshot);
 extern void UpdateActiveSnapshotCommandId(void);
 extern void PopActiveSnapshot(void);

diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 4c9b155015df1a8513b216d4fa92f2578639324b..df5aaf6ef74a74aae32f44b1588b9f8bfed2e317 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -430,6 +430,34 @@ SELECT * FROM test1;
 ---+---
 (0 rows)
 
+-- test commit/rollback inside exception handler, too
+TRUNCATE test1;
+DO LANGUAGE plpgsql $$
+BEGIN
+    FOR i IN 1..10 LOOP
+      BEGIN
+        INSERT INTO test1 VALUES (i, 'good');
+        INSERT INTO test1 VALUES (i/0, 'bad');
+      EXCEPTION
+        WHEN division_by_zero THEN
+            INSERT INTO test1 VALUES (i, 'exception');
+            IF (i % 3) > 0 THEN COMMIT; ELSE ROLLBACK; END IF;
+      END;
+    END LOOP;
+END;
+$$;
+SELECT * FROM test1;
+ a  |     b     
+----+-----------
+  1 | exception
+  2 | exception
+  4 | exception
+  5 | exception
+  7 | exception
+  8 | exception
+ 10 | exception
+(7 rows)
+
 -- detoast result of simple expression after commit
 CREATE TEMP TABLE test4(f1 text);
 ALTER TABLE test4 ALTER COLUMN f1 SET STORAGE EXTERNAL; -- disable compression

diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index f43061d7bae0448ff447bf3bac83ba8a63868f83..71df6d3f1ce2882c3c5b46536d056158632701fa 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -354,6 +354,27 @@ $$;
 SELECT * FROM test1;
 
 
+-- test commit/rollback inside exception handler, too
+TRUNCATE test1;
+
+DO LANGUAGE plpgsql $$
+BEGIN
+    FOR i IN 1..10 LOOP
+      BEGIN
+        INSERT INTO test1 VALUES (i, 'good');
+        INSERT INTO test1 VALUES (i/0, 'bad');
+      EXCEPTION
+        WHEN division_by_zero THEN
+            INSERT INTO test1 VALUES (i, 'exception');
+            IF (i % 3) > 0 THEN COMMIT; ELSE ROLLBACK; END IF;
+      END;
+    END LOOP;
+END;
+$$;
+
+SELECT * FROM test1;
+
+
 -- detoast result of simple expression after commit
 CREATE TEMP TABLE test4(f1 text);
 ALTER TABLE test4 ALTER COLUMN f1 SET STORAGE EXTERNAL; -- disable compression