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 7b53feb4432cdf5293adb2d540bd9bf05df3e118..ff84b5891059f9e965f435e9ff8c019f9ddb2d33 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.113.2.3 2010/02/18 03:06:53 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/mmgr/portalmem.c,v 1.113.2.4 2010/07/05 09:27:24 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -376,6 +376,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.
@@ -385,9 +407,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 not
@@ -630,6 +659,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).
@@ -738,7 +774,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 c19867310a342d8114f24b3fedd102f7a3e8dbd6..ca8546c1e5abcbab028a77510d9acb35a432810a 100644 (file)
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -39,7 +39,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.81 2009/01/01 17:24:02 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/portal.h,v 1.81.2.1 2010/07/05 09:27:24 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -133,6 +133,7 @@ typedef struct PortalData
 
        /* Status data */
        PortalStatus status;            /* see above */
+       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 */
@@ -199,6 +200,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 Portal GetPortalByName(const char *name);
 extern void PortalDefineQuery(Portal portal,

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 2931b2c4f79081db0b00de81d3272e0cfb1a3ddb..04f3638510200903c89c8beb3cd3e16ee6e7ef02 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.244.2.5 2010/06/21 09:49:58 heikki Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.244.2.6 2010/07/05 09:27:24 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1869,7 +1869,6 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
 {
        PLpgSQL_var *curvar;
        char       *curname = NULL;
-       const char   *portalname;
        PLpgSQL_expr *query;
        Portal          portal;
        int                     rc;
@@ -1952,7 +1951,6 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
        if (portal == NULL)
                elog(ERROR, "could not open cursor: %s",
                         SPI_result_code_string(SPI_result));
-       portalname = portal->name;
 
        /*
         * If cursor variable was NULL, store the generated portal name in it
@@ -1967,20 +1965,11 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
        rc = exec_for_query(estate, (PLpgSQL_stmt_forq *) stmt, portal, false);
 
        /* ----------
-        * Close portal. The statements executed in the loop might've closed the
-        * cursor already, rendering our portal pointer invalid, so we mustn't
-        * trust the pointer.
+        * Close portal, and restore cursor variable if it was initially NULL.
         * ----------
         */
-       portal = SPI_cursor_find(portalname);
-       if (portal == NULL)
-               ereport(ERROR,
-                               (errcode(ERRCODE_UNDEFINED_CURSOR),
-                                errmsg("cursor \"%s\" closed unexpectedly",
-                                               portalname)));
        SPI_cursor_close(portal);
 
-       /* Restore cursor variable if it was initially NULL. */
        if (curname == NULL)
        {
                free_var(curvar);
@@ -4200,13 +4189,6 @@ exec_run_select(PLpgSQL_execstate *estate,
  * exec_for_query --- execute body of FOR loop for each row from a portal
  *
  * Used by exec_stmt_fors, exec_stmt_forc and exec_stmt_dynfors
- *
- * If the portal is for a cursor that's visible to user code, the statements
- * we execute might move or close the cursor. You must pass prefetch_ok=false
- * in that case to disable optimizations that rely on the portal staying
- * unchanged over execution of the user statements.
- * NB: With prefetch_ok=false, the portal pointer might point to garbage
- * after the call. Caller beware!
  */
 static int
 exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
@@ -4218,10 +4200,6 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
        bool            found = false;
        int                     rc = PLPGSQL_RC_OK;
        int                     n;
-       const char *portalname;
-
-       /* Remember portal name so that we can re-find it */
-       portalname = portal->name;
 
        /*
         * Determine if we assign to a record or a row
@@ -4233,6 +4211,12 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
        else
                elog(ERROR, "unsupported target");
 
+       /*
+        * Make sure the portal doesn't get closed by the user statements
+        * we execute.
+        */
+       PinPortal(portal);
+
        /*
         * Fetch the initial tuple(s).  If prefetching is allowed then we grab a
         * few more rows to avoid multiple trips through executor startup
@@ -4330,22 +4314,8 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt,
 
                /*
                 * Fetch more tuples.  If prefetching is allowed, grab 50 at a time.
-                * Otherwise the statements executed in the loop might've moved or
-                * even closed the cursor, so check that the cursor is still open,
-                * and fetch only one row at a time.
                 */
-               if (prefetch_ok)
-                       SPI_cursor_fetch(portal, true, 50);
-               else
-               {
-                       portal = SPI_cursor_find(portalname);
-                       if (portal == NULL)
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_UNDEFINED_CURSOR),
-                                                errmsg("cursor \"%s\" closed unexpectedly",
-                                                               portalname)));
-                       SPI_cursor_fetch(portal, true, 1);
-               }
+               SPI_cursor_fetch(portal, true, prefetch_ok ? 50 : 1);
                tuptab = SPI_tuptable;
                n = SPI_processed;
        }
@@ -4357,6 +4327,8 @@ loop_exit:
         */
        SPI_freetuptable(tuptab);
 
+       UnpinPortal(portal);
+
        /*
         * Set the FOUND variable to indicate the result of executing the loop
         * (namely, whether we looped one or more times). This must be set last so