The previous fix in CVS HEAD and 8.4 for handling the case where a cursor

being used in a PL/pgSQL FOR loop is closed was inadequate, as Tom Lane
pointed out. The bug affects FOR statement variants too, because you can
close an implicitly created cursor too by guessing the "<unnamed portal X>"
name created for it.

To fix that, "pin" the portal to prevent it from being dropped while it's
being used in a PL/pgSQL FOR loop. Backpatch all the way to 7.4 which is
the oldest supported version.

diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index f6156e63c37019eee58367c3a30809be80cd253e..bb8931c8a7a6260edfba7dd9a0ebab684c2077f5 100644 (file)
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.76.4.2 2005/04/11 19:51:31 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.76.4.3 2010/07/05 09:27:49 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -295,6 +295,28 @@ PortalCreateHoldStore(Portal portal)
        MemoryContextSwitchTo(oldcxt);
 }
 
+/*
+ * PinPortal
+ *             Protect a portal from dropping.
+ */
+void
+PinPortal(Portal portal)
+{
+       if (portal->portalPinned)
+               elog(ERROR, "portal already pinned");
+
+       portal->portalPinned = true;
+}
+
+void
+UnpinPortal(Portal portal)
+{
+       if (!portal->portalPinned)
+               elog(ERROR, "portal not pinned");
+
+       portal->portalPinned = false;
+}
+
 /*
  * PortalDrop
  *             Destroy the portal.
@@ -304,9 +326,16 @@ PortalDrop(Portal portal, bool isTopCommit)
 {
        AssertArg(PortalIsValid(portal));
 
-       /* Not sure if this case can validly happen or not... */
-       if (portal->status == PORTAL_ACTIVE)
-               elog(ERROR, "cannot drop active portal");
+       /*
+        * Don't allow dropping a pinned portal, it's still needed by whoever
+        * pinned it. Not sure if the PORTAL_ACTIVE case can validly happen or
+        * not...
+        */
+       if (portal->portalPinned ||
+               portal->status == PORTAL_ACTIVE)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_CURSOR_STATE),
+                                errmsg("cannot drop active portal \"%s\"", portal->name)));
 
        /*
         * Remove portal from hash table.  Because we do this first, we will
@@ -507,6 +536,13 @@ AtCommit_Portals(void)
                        continue;
                }
 
+               /*
+                * There should be no pinned portals anymore. Complain if someone
+                * leaked one.
+                */
+               if (portal->portalPinned)
+                       elog(ERROR, "cannot commit while a portal is pinned");
+
                /*
                 * Do nothing to cursors held over from a previous transaction
                 * (including holdable ones just frozen by CommitHoldablePortals).
@@ -590,7 +626,15 @@ AtCleanup_Portals(void)
                        continue;
                }
 
-               /* Else zap it. */
+               /*
+                * If a portal is still pinned, forcibly unpin it. PortalDrop will
+                * not let us drop the portal otherwise. Whoever pinned the portal
+                * was interrupted by the abort too and won't try to use it anymore.
+                */
+               if (portal->portalPinned)
+                       portal->portalPinned = false;
+
+               /* Zap it. */
                PortalDrop(portal, false);
        }
 }

diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index c1ab891955ce81914b71f31cc97f5fad984875ce..afc514901940555ec1d48ae17852f34b05cc3103 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -39,7 +39,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.54.4.1 2005/04/11 19:51:32 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.54.4.2 2010/07/05 09:27:49 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -135,6 +135,7 @@ typedef struct PortalData
        /* Status data */
        PortalStatus status;            /* see above */
        bool            portalUtilReady;        /* PortalRunUtility complete? */
+       bool            portalPinned;   /* a pinned portal can't be dropped */
 
        /* If not NULL, Executor is active; call ExecutorEnd eventually: */
        QueryDesc  *queryDesc;          /* info needed for executor invocation */
@@ -195,6 +196,8 @@ extern void AtSubAbort_Portals(SubTransactionId mySubid,
 extern void AtSubCleanup_Portals(SubTransactionId mySubid);
 extern Portal CreatePortal(const char *name, bool allowDup, bool dupSilent);
 extern Portal CreateNewPortal(void);
+extern void PinPortal(Portal portal);
+extern void UnpinPortal(Portal portal);
 extern void PortalDrop(Portal portal, bool isTopCommit);
 extern void DropDependentPortals(MemoryContext queryContext);
 extern Portal GetPortalByName(const char *name);

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index db2f3d0545d8ff0a88c17aec48595b4e0a47a86e..985f4c908052a9cfea068b796b2bdd314f311050 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.127.4.8 2010/02/12 19:38:15 tgl Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.127.4.9 2010/07/05 09:27:49 heikki Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -1525,9 +1525,11 @@ exec_stmt_fors(PLpgSQL_execstate *estate, PLpgSQL_stmt_fors *stmt)
 
        /*
         * Open the implicit cursor for the statement and fetch the initial 10
-        * rows.
+        * rows. Pin the portal to make sure it doesn't get closed by the user
+        * statements we execute.
         */
        exec_run_select(estate, stmt->query, 0, &portal);
+       PinPortal(portal);
 
        SPI_cursor_fetch(portal, true, 10);
        tuptab = SPI_tuptable;
@@ -1566,6 +1568,7 @@ exec_stmt_fors(PLpgSQL_execstate *estate, PLpgSQL_stmt_fors *stmt)
                                 * (This code should match the code after the loop.)
                                 */
                                SPI_freetuptable(tuptab);
+                               UnpinPortal(portal);
                                SPI_cursor_close(portal);
                                exec_set_found(estate, found);
 
@@ -1612,6 +1615,7 @@ exec_stmt_fors(PLpgSQL_execstate *estate, PLpgSQL_stmt_fors *stmt)
        /*
         * Close the implicit cursor
         */
+       UnpinPortal(portal);
        SPI_cursor_close(portal);
 
        /*
@@ -2415,6 +2419,12 @@ exec_stmt_dynfors(PLpgSQL_execstate *estate, PLpgSQL_stmt_dynfors *stmt)
        pfree(querystr);
        SPI_freeplan(plan);
 
+       /*
+        * Make sure the portal doesn't get closed by the user statements
+        * we execute.
+        */
+       PinPortal(portal);
+
        /*
         * Fetch the initial 10 tuples
         */
@@ -2455,6 +2465,7 @@ exec_stmt_dynfors(PLpgSQL_execstate *estate, PLpgSQL_stmt_dynfors *stmt)
                                 * (This code should match the code after the loop.)
                                 */
                                SPI_freetuptable(tuptab);
+                               UnpinPortal(portal);
                                SPI_cursor_close(portal);
                                exec_set_found(estate, found);
 
@@ -2501,6 +2512,7 @@ exec_stmt_dynfors(PLpgSQL_execstate *estate, PLpgSQL_stmt_dynfors *stmt)
        /*
         * Close the implicit cursor
         */
+       UnpinPortal(portal);
        SPI_cursor_close(portal);
 
        /*