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 ad12395bf2f7140d5d9c47d84b55e4c78bf7f6a4..278d747e21cbf13a6ba9fc217b16e08f607327f0 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4842,6 +4842,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 5781dcde7c605c7e8453d80c3df5887bc3e945b1..a8f65ca1658fa4170ae3a1afc056c92a13def8da 100644 (file)
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -480,7 +480,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.
                 */
 
                /*
@@ -1134,9 +1136,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
@@ -1786,8 +1794,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 a34abbd80c45296c6c3fc48b6294bb9a5157e122..11ee752f84ac1758dad198b360664368414ca020 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;
@@ -657,6 +658,7 @@ HoldPortal(Portal portal)
     */
    portal->createSubid = InvalidSubTransactionId;
    portal->activeSubid = InvalidSubTransactionId;
+   portal->createLevel = 0;
 }
 
 /*
@@ -940,6 +942,7 @@ PortalErrorCleanup(void)
 void
 AtSubCommit_Portals(SubTransactionId mySubid,
                    SubTransactionId parentSubid,
+                   int parentLevel,
                    ResourceOwner parentXactOwner)
 {
    HASH_SEQ_STATUS status;
@@ -954,6 +957,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 5e3b31ba9027dcfc5268d65655f731357c977f19..1c960300fd28de4d11831720e51b164625ce6ac2 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -733,10 +733,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));
 
@@ -750,7 +765,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 cb40bfa761aeadf133e07291d60a1a62e695e4f4..af0afcf98560a78677104958f3748142ce2b9e04 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -195,6 +195,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.
@@ -202,6 +204,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;
 
 /*
@@ -219,6 +222,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 ad7c15dc1a2e25fdb42d239a5137f95cc4c186e0..651ff6099821a0252fa43fbbb9f6191dc486cfd3 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -110,6 +110,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 ab49db3f34361ecbce0d673c4f008677015c0edc..09a7e311e6e4aa5f7bec0a3ab97fe5c4443121ab 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 888ddccaceba6f9487bcc55e4873c2219971ad4f..8d76d00daaab82540890add2d292c168abdb2aa8 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