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 6597ec45a953ca4aa8ba1c6f24d43a47bc3d52e8..4cc38f0d85e652e94f5f731727752ae3062676a5 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -4863,6 +4863,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 b5797042abc46be8a098e317213a88f2f2af033f..960f3fadce54087afc361dd3fff8d8120e7e3695 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.
                 */
 
                /*
@@ -1136,9 +1138,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
@@ -1784,8 +1792,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 5c30e141f52cf7186c62442495f39b429c6b8ce6..58674d611d477b44eb3f90976bc5f026ca011f0e 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 2968c7f7b7dc0930a9071f2e20d258a31026eecd..dca1bc8afca6e4f52e1bf3a3fda7c7f00fca4a1a 100644 (file)
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -678,10 +678,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));
 
@@ -695,7 +710,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 2e5bbdd0ec4881a9aaa247f989cd53697f922b06..37b1817ae91af88498bc51b7eb71205d391f006e 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -130,6 +130,7 @@ typedef struct PortalData
     */
    SubTransactionId createSubid;   /* the creating subxact */
    SubTransactionId activeSubid;   /* the last subxact with activity */
+   int         createLevel;    /* creating subxact's nesting level */
 
    /* The query or queries the portal will execute */
    const char *sourceText;     /* text of query (as of 8.4, never NULL) */
@@ -219,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 44539fe15ab4aa94bf3d742bb62ac96bc1c3fb14..c6a176cc95de4335020682b7583977d9d17feb0d 100644 (file)
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -114,6 +114,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 f79f847321c986d47571425de17ff09957a6eeee..254e5b7a7067949fa38d9532c1c36b7d77d2a9fa 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