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 00bf3e23d2395f586551b7c33177e75f8ac2e34a..00c4cf6a1083304ebbdb8eb096844c7b079afcbc 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
- *       $Header: /cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v 1.62 2003/08/24 21:02:43 petere Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v 1.62.2.1 2010/07/05 09:27:56 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -288,6 +288,27 @@ PortalCreateHoldStore(Portal portal)
        MemoryContextSwitchTo(oldcxt);
 }
 
+/*
+ * PinPortal
+ *             Protect a portal from dropping.
+ */
+void
+PinPortal(Portal portal)
+{
+       if (!portal->portalReady)
+               elog(ERROR, "cannot pin portal that's not in ready state");
+
+       portal->portalPinned = true;
+}
+
+void
+UnpinPortal(Portal portal)
+{
+       if (!portal->portalPinned)
+               elog(ERROR, "portal not pinned");
+       portal->portalPinned = false;
+}
+
 /*
  * PortalDrop
  *             Destroy the portal.
@@ -300,9 +321,17 @@ PortalDrop(Portal portal, bool isError)
 {
        AssertArg(PortalIsValid(portal));
 
-       /* Not sure if this case can validly happen or not... */
-       if (portal->portalActive)
-               elog(ERROR, "cannot drop active portal");
+       /*
+        * Don't allow dropping a pinned portal, it's still needed by whoever
+        * pinned it. Unless we're doing post-abort cleanup; whoever pinned the
+        * portal is going to go away at transaction abort anyway.
+        *
+        * Not sure if the portalActive case can validly happen or not...
+        */
+       if ((portal->portalPinned && !isError) || portal->portalActive)
+               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
@@ -400,6 +429,13 @@ AtCommit_Portals(void)
                if (portal->portalActive)
                        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");
+
                if (portal->cursorOptions & CURSOR_OPT_HOLD)
                {
                        /*

diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index 3faf3ec562fb9252b83fe61902e86650e263edf2..2006c4ce9700e557694ed4c342a2b81f9d973c95 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -39,7 +39,7 @@
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: portal.h,v 1.47 2003/08/08 21:42:55 momjian Exp $
+ * $Id: portal.h,v 1.47.4.1 2010/07/05 09:27:56 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -117,6 +117,7 @@ typedef struct PortalData
        bool            portalUtilReady;        /* PortalRunUtility complete? */
        bool            portalActive;   /* portal is running (can't delete it) */
        bool            portalDone;             /* portal is finished (don't re-run it) */
+       bool            portalPinned;   /* portal is pinned (can't delete it) */
 
        /* If not NULL, Executor is active; call ExecutorEnd eventually: */
        QueryDesc  *queryDesc;          /* info needed for executor invocation */
@@ -169,6 +170,8 @@ extern void AtAbort_Portals(void);
 extern void AtCleanup_Portals(void);
 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 isError);
 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 8cba7101a3789f5302be8098d564e1557a2f7dd8..c58fba0b568f516c8874a36a789e21594c2a9839 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
- *       $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.93.2.3 2007/02/08 18:38:19 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.93.2.4 2010/07/05 09:27:57 heikki Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -1384,9 +1384,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;
@@ -1425,6 +1427,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);
 
@@ -1471,6 +1474,7 @@ exec_stmt_fors(PLpgSQL_execstate * estate, PLpgSQL_stmt_fors * stmt)
        /*
         * Close the implicit cursor
         */
+       UnpinPortal(portal);
        SPI_cursor_close(portal);
 
        /*
@@ -2227,6 +2231,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
         */
@@ -2267,6 +2277,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);
 
@@ -2313,6 +2324,7 @@ exec_stmt_dynfors(PLpgSQL_execstate * estate, PLpgSQL_stmt_dynfors * stmt)
        /*
         * Close the implicit cursor
         */
+       UnpinPortal(portal);
        SPI_cursor_close(portal);
 
        /*