Code review for holdable-cursors patch. Fix error recovery, memory

context sloppiness, some other things.  Includes Neil's mopup patch
of 22-Apr.

doc/src/sgml/ref/declare.sgml

 <!--
-$Header: /cvsroot/pgsql/doc/src/sgml/ref/declare.sgml,v 1.22 2003/04/06 22:41:52 petere Exp $
+$Header: /cvsroot/pgsql/doc/src/sgml/ref/declare.sgml,v 1.23 2003/04/29 03:21:28 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -70,8 +70,8 @@ DECLARE <replaceable class="parameter">cursorname</replaceable> [ BINARY ] [ INS
       <term>NO SCROLL</term>
       <listitem>
        <para>
-    Specifies that the cursor cannot be used to retrieve rows in a
-    nonsequential fashion (e.g., backward).
+        Specifies that the cursor cannot be used to retrieve rows in a
+        nonsequential fashion (e.g., backward).
        </para>
       </listitem>
      </varlistentry>
@@ -83,7 +83,7 @@ DECLARE <replaceable class="parameter">cursorname</replaceable> [ BINARY ] [ INS
 	Specifies that the cursor may be used to retrieve rows in a
 	nonsequential fashion (e.g., backward). Depending upon the
 	complexity of the query's execution plan, specifying
-	<literal>SCROLL</literal> may impose a slight performance penalty
+	<literal>SCROLL</literal> may impose a performance penalty
 	on the query's execution time.
        </para>
       </listitem>
@@ -96,7 +96,7 @@ DECLARE <replaceable class="parameter">cursorname</replaceable> [ BINARY ] [ INS
         Specifies that the cursor cannot be used outside of the
         transaction that created it. If neither <literal>WITHOUT
         HOLD</literal> nor <literal>WITH HOLD</literal> is specified,
-        <literal>WITH HOLD</literal> is the default.
+        <literal>WITHOUT HOLD</literal> is the default.
        </para>
       </listitem>
      </varlistentry>
@@ -105,8 +105,8 @@ DECLARE <replaceable class="parameter">cursorname</replaceable> [ BINARY ] [ INS
       <term>WITH HOLD</term>
       <listitem>
        <para>
-        Specifies that the cursor may be used after the transaction
-        that creates it successfully commits.
+        Specifies that the cursor may continue to be used after the
+	transaction that creates it successfully commits.
        </para>
       </listitem>
      </varlistentry>
@@ -163,7 +163,7 @@ DECLARE <replaceable class="parameter">cursorname</replaceable> [ BINARY ] [ INS
 
    <para>
     The <literal>BINARY</literal>, <literal>INSENSITIVE</literal>,
-    <literal>SCROLL</literal> keywords may appear in any order.
+    and <literal>SCROLL</literal> keywords may appear in any order.
    </para>
   </refsect2>
 
@@ -296,11 +296,14 @@ ERROR:  DECLARE CURSOR may only be used in begin/end transaction blocks
    <para>
     If <literal>WITH HOLD</literal> is specified and the transaction
     that created the cursor successfully commits, the cursor can be
-    accessed outside the creating transaction. If the creating
-    transaction is aborted, the cursor is removed. A cursor created
+    continue to be accessed by subsequent transactions in the same session.
+    (But if the creating
+    transaction is aborted, the cursor is removed.) A cursor created
     with <literal>WITH HOLD</literal> is closed when an explicit
-    <command>CLOSE</command> command is issued on it, or the client
-    connection is terminated.
+    <command>CLOSE</command> command is issued on it, or when the client
+    connection is terminated.  In the current implementation, the rows
+    represented by a held cursor are copied into a temporary file or
+    memory area so that they remain available for subsequent transactions.
    </para>
 
    <para>
@@ -312,7 +315,8 @@ ERROR:  DECLARE CURSOR may only be used in begin/end transaction blocks
     plan is simple enough that no extra overhead is needed to support
     it. However, application developers are advised not to rely on
     using backward fetches from a cursor that has not been created
-    with <literal>SCROLL</literal>.
+    with <literal>SCROLL</literal>.  If <literal>NO SCROLL</> is specified,
+    then backward fetches are disallowed in any case.
    </para>
 
    <para>

src/backend/catalog/sql_features.txt

@@ -95,7 +95,7 @@ E121	Basic cursor support	06	Positioned UPDATE statement	NO
 E121	Basic cursor support	07	Positioned DELETE statement	NO	
 E121	Basic cursor support	08	CLOSE statement	YES	
 E121	Basic cursor support	10	FETCH statement implicit NEXT	YES	
-E121	Basic cursor support	17	WITH HOLD cursors	NO	
+E121	Basic cursor support	17	WITH HOLD cursors	YES	
 E131	Null value support (nulls in lieu of values)			YES	
 E141	Basic integrity constraints			YES	
 E141	Basic integrity constraints	01	NOT NULL constraints	YES	
@@ -214,12 +214,12 @@ F401	Extended joined table	03	UNION JOIN	YES
 F401	Extended joined table	04	CROSS JOIN	YES	
 F411	Time zone specification			YES	
 F421	National character			YES	
-F431	Read-only scrollable cursors			NO	
+F431	Read-only scrollable cursors			YES	
 F431	Read-only scrollable cursors	01	FETCH with explicit NEXT	YES	
-F431	Read-only scrollable cursors	02	FETCH FIRST	NO	
-F431	Read-only scrollable cursors	03	FETCH LAST	NO	
+F431	Read-only scrollable cursors	02	FETCH FIRST	YES	
+F431	Read-only scrollable cursors	03	FETCH LAST	YES	
 F431	Read-only scrollable cursors	04	FETCH PRIOR	YES	
-F431	Read-only scrollable cursors	05	FETCH ABSOLUTE	NO	
+F431	Read-only scrollable cursors	05	FETCH ABSOLUTE	YES	
 F431	Read-only scrollable cursors	06	FETCH RELATIVE	YES	
 F441	Extended set function support			YES	
 F451	Character set definition			NO	
@@ -319,7 +319,7 @@ T211	Basic trigger capability	04	FOR EACH ROW triggers	YES
 T211	Basic trigger capability	05	Ability to specify a search condition that must be true before the trigger is invoked	NO	
 T211	Basic trigger capability	06	Support for run-time rules for the interaction of triggers and constraints	NO	
 T211	Basic trigger capability	07	TRIGGER privilege	YES	
-T211	Basic trigger capability	08	Multiple triggers for the same the event are executed in the order in which they were created	NO	
+T211	Basic trigger capability	08	Multiple triggers for the same event are executed in the order in which they were created	NO	intentionally omitted
 T212	Enhanced trigger capability			YES	
 T231	SENSITIVE cursors			YES	
 T241	START TRANSACTION statement			YES	

src/backend/executor/spi.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/executor/spi.c,v 1.91 2003/04/27 20:09:43 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/executor/spi.c,v 1.92 2003/04/29 03:21:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -809,7 +809,7 @@ SPI_cursor_open(const char *name, void *plan, Datum *Values, const char *Nulls)
 	ExecutorStart(queryDesc);
 
 	/* Arrange to shut down the executor if portal is dropped */
-	PortalSetQuery(portal, queryDesc, PortalCleanup);
+	PortalSetQuery(portal, queryDesc);
 
 	/* Switch back to the callers memory context */
 	MemoryContextSwitchTo(oldcontext);

src/backend/executor/tstoreReceiver.c

@@ -9,7 +9,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/executor/tstoreReceiver.c,v 1.1 2003/03/27 16:53:15 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/executor/tstoreReceiver.c,v 1.2 2003/04/29 03:21:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -27,8 +27,9 @@ typedef struct
 	MemoryContext		cxt;
 } TStoreState;
 
+
 /*
- * Receive a tuple from the executor and store it in the tuplestore.
+ * Prepare to receive tuples from executor.
  *
  * XXX: As currently implemented, this routine is a hack: there should
  * be no tie between this code and the portal system. Instead, the
@@ -56,6 +57,9 @@ tstoreSetupReceiver(DestReceiver *self, int operation,
 	myState->cxt = portal->holdContext;
 }
 
+/*
+ * Receive a tuple from the executor and store it in the tuplestore.
+ */
 static void
 tstoreReceiveTuple(HeapTuple tuple, TupleDesc typeinfo, DestReceiver *self)
 {
@@ -67,12 +71,18 @@ tstoreReceiveTuple(HeapTuple tuple, TupleDesc typeinfo, DestReceiver *self)
 	MemoryContextSwitchTo(oldcxt);
 }
 
+/*
+ * Clean up
+ */
 static void
 tstoreCleanupReceiver(DestReceiver *self)
 {
-	; /* do nothing */
+	/* do nothing */
 }
 
+/*
+ * Initially create a DestReceiver object.
+ */
 DestReceiver *
 tstoreReceiverCreateDR(void)
 {

src/backend/parser/analyze.c

@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- *	$Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.266 2003/03/27 16:51:28 momjian Exp $
+ *	$Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.267 2003/04/29 03:21:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2299,7 +2299,7 @@ transformDeclareCursorStmt(ParseState *pstate, DeclareCursorStmt *stmt)
 	 */
 	if ((stmt->options & CURSOR_OPT_SCROLL) &&
 		(stmt->options & CURSOR_OPT_NO_SCROLL))
-		elog(ERROR, "Both SCROLL and NO SCROLL cannot be specified.");
+		elog(ERROR, "Cannot specify both SCROLL and NO SCROLL");
 
 	stmt->query = (Node *) transformStmt(pstate, stmt->query,
 										 &extras_before, &extras_after);

src/backend/parser/gram.y

@@ -11,7 +11,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.411 2003/04/08 23:20:01 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/parser/gram.y,v 2.412 2003/04/29 03:21:29 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -4236,10 +4236,8 @@ DeclareCursorStmt: DECLARE name cursor_options CURSOR opt_hold FOR SelectStmt
 					n->portalname = $2;
 					n->options = $3;
 					n->query = $7;
-
 					if ($5)
 						n->options |= CURSOR_OPT_HOLD;
-
 					$$ = (Node *)n;
 				}
 		;
@@ -7191,6 +7189,7 @@ unreserved_keyword:
 			| FUNCTION
 			| GLOBAL
 			| HANDLER
+			| HOLD
 			| HOUR_P
 			| IMMEDIATE
 			| IMMUTABLE

src/backend/storage/file/buffile.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/file/buffile.c,v 1.15 2003/03/27 16:51:29 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/file/buffile.c,v 1.16 2003/04/29 03:21:29 tgl Exp $
  *
  * NOTES:
  *
@@ -64,7 +64,7 @@ struct BufFile
 	 */
 
 	bool		isTemp;			/* can only add files if this is TRUE */
-	bool		isInterTxn;		/* keep open over transactions? */
+	bool		isInterXact;	/* keep open over transactions? */
 	bool		dirty;			/* does buffer need to be written? */
 
 	/*
@@ -119,7 +119,7 @@ extendBufFile(BufFile *file)
 	File		pfile;
 
 	Assert(file->isTemp);
-	pfile = OpenTemporaryFile(file->isInterTxn);
+	pfile = OpenTemporaryFile(file->isInterXact);
 	Assert(pfile >= 0);
 
 	file->files = (File *) repalloc(file->files,
@@ -135,19 +135,22 @@ extendBufFile(BufFile *file)
  * Create a BufFile for a new temporary file (which will expand to become
  * multiple temporary files if more than MAX_PHYSICAL_FILESIZE bytes are
  * written to it).
+ *
+ * Note: if interXact is true, the caller had better be calling us in a
+ * memory context that will survive across transaction boundaries.
  */
 BufFile *
-BufFileCreateTemp(bool interTxn)
+BufFileCreateTemp(bool interXact)
 {
 	BufFile    *file;
 	File		pfile;
 
-	pfile = OpenTemporaryFile(interTxn);
+	pfile = OpenTemporaryFile(interXact);
 	Assert(pfile >= 0);
 
 	file = makeBufFile(pfile);
 	file->isTemp = true;
-	file->isInterTxn = interTxn;
+	file->isInterXact = interXact;
 
 	return file;
 }

src/backend/storage/file/fd.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/storage/file/fd.c,v 1.97 2003/04/04 20:42:12 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/storage/file/fd.c,v 1.98 2003/04/29 03:21:29 tgl Exp $
  *
  * NOTES:
  *
@@ -113,8 +113,8 @@ int			max_files_per_process = 1000;
 #define FileUnknownPos (-1L)
 
 /* these are the assigned bits in fdstate below: */
-#define FD_TEMPORARY		(1 << 0)
-#define FD_TXN_TEMPORARY	(1 << 1)
+#define FD_TEMPORARY		(1 << 0)		/* T = delete when closed */
+#define FD_XACT_TEMPORARY	(1 << 1)		/* T = delete at eoXact */
 
 typedef struct vfd
 {
@@ -156,7 +156,7 @@ static int	numAllocatedFiles = 0;
 static FILE *allocatedFiles[MAX_ALLOCATED_FILES];
 
 /*
- * Number of temporary files opened during the current transaction;
+ * Number of temporary files opened during the current session;
  * this is used in generation of tempfile names.
  */
 static long tempFileCounter = 0;
@@ -205,6 +205,9 @@ static int	FileAccess(File file);
 static File fileNameOpenFile(FileName fileName, int fileFlags, int fileMode);
 static char *filepath(const char *filename);
 static long pg_nofile(void);
+static void AtProcExit_Files(void);
+static void CleanupTempFiles(bool isProcExit);
+
 
 /*
  * pg_fsync --- same as fsync except does nothing if enableFsync is off
@@ -522,7 +525,7 @@ AllocateVfd(void)
 		 * register proc-exit call to ensure temp files are dropped at
 		 * exit
 		 */
-		on_proc_exit(AtEOXact_Files, 0);
+		on_proc_exit(AtProcExit_Files, 0);
 	}
 
 	if (VfdCache[0].nextFree == 0)
@@ -751,21 +754,21 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
  * There's no need to pass in fileFlags or fileMode either, since only
  * one setting makes any sense for a temp file.
  *
- * keepOverTxn: if true, don't close the file at end-of-transaction. In
+ * interXact: if true, don't close the file at end-of-transaction. In
  * most cases, you don't want temporary files to outlive the transaction
  * that created them, so this should be false -- but if you need
  * "somewhat" temporary storage, this might be useful. In either case,
- * the file is removed when the File is explicitely closed.
+ * the file is removed when the File is explicitly closed.
  */
 File
-OpenTemporaryFile(bool keepOverTxn)
+OpenTemporaryFile(bool interXact)
 {
-	char		tempfilepath[128];
+	char		tempfilepath[MAXPGPATH];
 	File		file;
 
 	/*
-	 * Generate a tempfile name that's unique within the current
-	 * transaction and database instance.
+	 * Generate a tempfile name that should be unique within the current
+	 * database instance.
 	 */
 	snprintf(tempfilepath, sizeof(tempfilepath),
 			 "%s/%s%d.%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX,
@@ -798,15 +801,16 @@ OpenTemporaryFile(bool keepOverTxn)
 								O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
 								0600);
 		if (file <= 0)
-			elog(ERROR, "Failed to create temporary file %s", tempfilepath);
+			elog(ERROR, "Failed to create temporary file %s: %m",
+				 tempfilepath);
 	}
 
 	/* Mark it for deletion at close */
 	VfdCache[file].fdstate |= FD_TEMPORARY;
 
 	/* Mark it for deletion at EOXact */
-	if (!keepOverTxn)
-		VfdCache[file].fdstate |= FD_TXN_TEMPORARY;
+	if (!interXact)
+		VfdCache[file].fdstate |= FD_XACT_TEMPORARY;
 
 	return file;
 }
@@ -1108,44 +1112,79 @@ closeAllVfds(void)
 /*
  * AtEOXact_Files
  *
- * This routine is called during transaction commit or abort or backend
- * exit (it doesn't particularly care which).  All still-open temporary-file
- * VFDs are closed, which also causes the underlying files to be deleted.
- * Furthermore, all "allocated" stdio files are closed.
+ * This routine is called during transaction commit or abort (it doesn't
+ * particularly care which).  All still-open per-transaction temporary file
+ * VFDs are closed, which also causes the underlying files to be
+ * deleted. Furthermore, all "allocated" stdio files are closed.
  */
 void
 AtEOXact_Files(void)
 {
-	Index		i;
+	CleanupTempFiles(false);
+}
+
+/*
+ * AtProcExit_Files
+ *
+ * on_proc_exit hook to clean up temp files during backend shutdown.
+ * Here, we want to clean up *all* temp files including interXact ones.
+ */
+static void
+AtProcExit_Files(void)
+{
+	CleanupTempFiles(true);
+}
+
+/*
+ * Close temporary files and delete their underlying files.
+ *
+ * isProcExit: if true, this is being called as the backend process is
+ * exiting. If that's the case, we should remove all temporary files; if
+ * that's not the case, we are being called for transaction commit/abort
+ * and should only remove transaction-local temp files.  In either case,
+ * also clean up "allocated" stdio files.
+ */
+static void
+CleanupTempFiles(bool isProcExit)
+{
+	Index i;
 
 	if (SizeVfdCache > 0)
 	{
 		Assert(FileIsNotOpen(0));		/* Make sure ring not corrupted */
 		for (i = 1; i < SizeVfdCache; i++)
 		{
-			if ((VfdCache[i].fdstate & FD_TEMPORARY) &&
-				(VfdCache[i].fdstate & FD_TXN_TEMPORARY) &&
-				VfdCache[i].fileName != NULL)
-				FileClose(i);
+			unsigned short fdstate = VfdCache[i].fdstate;
+
+			if ((fdstate & FD_TEMPORARY) && VfdCache[i].fileName != NULL)
+			{
+				/*
+				 * If we're in the process of exiting a backend process,
+				 * close all temporary files. Otherwise, only close
+				 * temporary files local to the current transaction.
+				 */
+				if (isProcExit || (fdstate & FD_XACT_TEMPORARY))
+					FileClose(i);
+			}
 		}
 	}
 
 	while (numAllocatedFiles > 0)
 		FreeFile(allocatedFiles[0]);
-
-	/*
-	 * Reset the tempfile name counter to 0; not really necessary, but
-	 * helps keep the names from growing unreasonably long.
-	 */
-	tempFileCounter = 0;
 }
 
 
 /*
- * Remove old temporary files
+ * Remove temporary files left over from a prior postmaster session
  *
  * This should be called during postmaster startup.  It will forcibly
  * remove any leftover files created by OpenTemporaryFile.
+ *
+ * NOTE: we could, but don't, call this during a post-backend-crash restart
+ * cycle.  The argument for not doing it is that someone might want to examine
+ * the temp files for debugging purposes.  This does however mean that
+ * OpenTemporaryFile had better allow for collision with an existing temp
+ * file name.
  */
 void
 RemovePgTempFiles(void)
@@ -1194,15 +1233,9 @@ RemovePgTempFiles(void)
 								strlen(PG_TEMP_FILE_PREFIX)) == 0)
 						unlink(rm_path);
 					else
-					{
-						/*
-						 * would prefer to use elog here, but it's not up
-						 * and running during postmaster startup...
-						 */
-						fprintf(stderr,
-								"Unexpected file found in temporary-files directory: %s\n",
-								rm_path);
-					}
+						elog(LOG,
+							 "Unexpected file found in temporary-files directory: %s",
+							 rm_path);
 				}
 				closedir(temp_dir);
 			}

src/backend/utils/mmgr/portalmem.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v 1.54 2003/03/27 16:51:29 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/mmgr/portalmem.c,v 1.55 2003/04/29 03:21:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -33,6 +33,7 @@
 
 #include "postgres.h"
 
+#include "commands/portalcmds.h"
 #include "executor/executor.h"
 #include "utils/hsearch.h"
 #include "utils/memutils.h"
@@ -156,14 +157,14 @@ GetPortalByName(const char *name)
 
 /*
  * PortalSetQuery
- *		Attaches a "query" to the specified portal. Note that in the
- *		case of DECLARE CURSOR, some Portal options have already been
- *		set based upon the parsetree of the original DECLARE statement.
+ *		Attaches a QueryDesc to the specified portal.  This should be
+ *		called only after successfully doing ExecutorStart for the query.
+ *
+ * Note that in the case of DECLARE CURSOR, some Portal options have
+ * already been set in portalcmds.c's PreparePortal().  This is grotty.
  */
 void
-PortalSetQuery(Portal portal,
-			   QueryDesc *queryDesc,
-			   void (*cleanup) (Portal portal))
+PortalSetQuery(Portal portal, QueryDesc *queryDesc)
 {
 	AssertArg(PortalIsValid(portal));
 
@@ -174,18 +175,15 @@ PortalSetQuery(Portal portal,
 	 */
 	if (portal->scrollType == DEFAULT_SCROLL)
 	{
-		bool backwardPlan;
-
-		backwardPlan = ExecSupportsBackwardScan(queryDesc->plantree);
-
-		if (backwardPlan)
+		if (ExecSupportsBackwardScan(queryDesc->plantree))
 			portal->scrollType = ENABLE_SCROLL;
 		else
 			portal->scrollType = DISABLE_SCROLL;
 	}
 
 	portal->queryDesc = queryDesc;
-	portal->cleanup = cleanup;
+	portal->executorRunning = true;	/* now need to shut down executor */
+
 	portal->atStart = true;
 	portal->atEnd = false;		/* allow fetches */
 	portal->portalPos = 0;
@@ -228,12 +226,13 @@ CreatePortal(const char *name)
 
 	/* initialize portal query */
 	portal->queryDesc = NULL;
-	portal->cleanup = NULL;
+	portal->cleanup = PortalCleanup;
 	portal->scrollType = DEFAULT_SCROLL;
+	portal->executorRunning = false;
 	portal->holdOpen = false;
+	portal->createXact = GetCurrentTransactionId();
 	portal->holdStore = NULL;
 	portal->holdContext = NULL;
-	portal->createXact = GetCurrentTransactionId();
 	portal->atStart = true;
 	portal->atEnd = true;		/* disallow fetches until query is set */
 	portal->portalPos = 0;
@@ -249,67 +248,34 @@ CreatePortal(const char *name)
  * PortalDrop
  *		Destroy the portal.
  *
- *		keepHoldable: if true, holdable portals should not be removed by
- *		this function. More specifically, invoking this function with
- *		keepHoldable = true on a holdable portal prepares the portal for
- *		access outside of its creating transaction.
+ *		isError: if true, we are destroying portals at the end of a failed
+ *		transaction.  (This causes PortalCleanup to skip unneeded steps.)
  */
 void
-PortalDrop(Portal portal, bool persistHoldable)
+PortalDrop(Portal portal, bool isError)
 {
 	AssertArg(PortalIsValid(portal));
 
-	if (portal->holdOpen && persistHoldable)
-	{
-		/*
-		 * We're "dropping" a holdable portal, but what we really need
-		 * to do is prepare the portal for access outside of its
-		 * creating transaction.
-		 */
-
-		/*
-		 * Create the memory context that is used for storage of
-		 * long-term (cross transaction) data needed by the holdable
-		 * portal.
-		 */
-		portal->holdContext =
-			AllocSetContextCreate(PortalMemory,
-								  "PortalHeapMemory",
-								  ALLOCSET_DEFAULT_MINSIZE,
-								  ALLOCSET_DEFAULT_INITSIZE,
-								  ALLOCSET_DEFAULT_MAXSIZE);
-
-		/*
-		 * Note that PersistHoldablePortal() releases any resources used
-		 * by the portal that are local to the creating txn.
-		 */
-		PersistHoldablePortal(portal);
-
-		return;
-	}
-
-	/* remove portal from hash table */
+	/*
+	 * Remove portal from hash table.  Because we do this first, we will
+	 * not come back to try to remove the portal again if there's any error
+	 * in the subsequent steps.  Better to leak a little memory than to get
+	 * into an infinite error-recovery loop.
+	 */
 	PortalHashTableDelete(portal);
 
-	/* reset portal */
+	/* let portalcmds.c clean up the state it knows about */
 	if (PointerIsValid(portal->cleanup))
-		(*portal->cleanup) (portal);
+		(*portal->cleanup) (portal, isError);
 
-	/*
-	 * delete short-term memory context; in the case of a holdable
-	 * portal, this has already been done
-	 */
-	if (PortalGetHeapMemory(portal))
-		MemoryContextDelete(PortalGetHeapMemory(portal));
-
-	/*
-	 * delete long-term memory context; in the case of a non-holdable
-	 * portal, this context has never been created, so we don't need to
-	 * do anything
-	 */
+	/* delete tuplestore storage, if any */
 	if (portal->holdContext)
 		MemoryContextDelete(portal->holdContext);
 
+	/* release subsidiary storage */
+	if (PortalGetHeapMemory(portal))
+		MemoryContextDelete(PortalGetHeapMemory(portal));
+
 	/* release name and portal data (both are in PortalMemory) */
 	pfree(portal->name);
 	pfree(portal);
@@ -320,8 +286,8 @@ PortalDrop(Portal portal, bool persistHoldable)
  * transaction was aborted, all the portals created in this transaction
  * should be removed. If the transaction was successfully committed, any
  * holdable cursors created in this transaction need to be kept
- * open. Only cursors created in the current transaction should be
- * removed in this fashion.
+ * open. In any case, portals remaining from prior transactions should
+ * be left untouched.
  *
  * XXX This assumes that portals can be deleted in a random order, ie,
  * no portal has a reference to any other (at least not one that will be
@@ -340,7 +306,42 @@ AtEOXact_portals(bool isCommit)
 
 	while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL)
 	{
-		if (hentry->portal->createXact == xact)
-			PortalDrop(hentry->portal, isCommit);
+		Portal portal = hentry->portal;
+
+		if (portal->createXact != xact)
+			continue;
+
+		if (portal->holdOpen && isCommit)
+		{
+			/*
+			 * We are exiting the transaction that created a holdable
+			 * cursor.  Instead of dropping the portal, prepare it for
+			 * access by later transactions.
+			 */
+
+			/*
+			 * Create the memory context that is used for storage of
+			 * the held cursor's tuple set.
+			 */
+			portal->holdContext =
+				AllocSetContextCreate(PortalMemory,
+									  "PortalHeapMemory",
+									  ALLOCSET_DEFAULT_MINSIZE,
+									  ALLOCSET_DEFAULT_INITSIZE,
+									  ALLOCSET_DEFAULT_MAXSIZE);
+
+			/*
+			 * Transfer data into the held tuplestore.
+			 *
+			 * Note that PersistHoldablePortal() must release all
+			 * resources used by the portal that are local to the creating
+			 * transaction.
+			 */
+			PersistHoldablePortal(portal);
+		}
+		else
+		{
+			PortalDrop(portal, !isCommit);
+		}
 	}
 }

src/backend/utils/sort/tuplestore.c

@@ -36,7 +36,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/utils/sort/tuplestore.c,v 1.12 2003/03/27 16:51:29 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/utils/sort/tuplestore.c,v 1.13 2003/04/29 03:21:29 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -65,7 +65,7 @@ struct Tuplestorestate
 {
 	TupStoreStatus status;		/* enumerated value as shown above */
 	bool		randomAccess;	/* did caller request random access? */
-	bool		interTxn;		/* keep open through transactions? */
+	bool		interXact;		/* keep open through transactions? */
 	long		availMem;		/* remaining memory available, in bytes */
 	BufFile    *myfile;			/* underlying file, or NULL if none */
 
@@ -191,7 +191,7 @@ struct Tuplestorestate
 
 
 static Tuplestorestate *tuplestore_begin_common(bool randomAccess,
-												bool interTxn,
+												bool interXact,
 												int maxKBytes);
 static void dumptuples(Tuplestorestate *state);
 static unsigned int getlen(Tuplestorestate *state, bool eofOK);
@@ -205,9 +205,8 @@ static void *readtup_heap(Tuplestorestate *state, unsigned int len);
  *
  * Initialize for a tuple store operation.
  */
-
 static Tuplestorestate *
-tuplestore_begin_common(bool randomAccess, bool interTxn, int maxKBytes)
+tuplestore_begin_common(bool randomAccess, bool interXact, int maxKBytes)
 {
 	Tuplestorestate *state;
 
@@ -215,7 +214,7 @@ tuplestore_begin_common(bool randomAccess, bool interTxn, int maxKBytes)
 
 	state->status = TSS_INMEM;
 	state->randomAccess = randomAccess;
-	state->interTxn = interTxn;
+	state->interXact = interXact;
 	state->availMem = maxKBytes * 1024L;
 	state->myfile = NULL;
 
@@ -244,17 +243,21 @@ tuplestore_begin_common(bool randomAccess, bool interTxn, int maxKBytes)
  * randomAccess: if true, both forward and backward accesses to the
  * tuple store are allowed.
  *
- * interTxn: if true, the files used by on-disk storage persist beyond
- * the end of the current transaction.
+ * interXact: if true, the files used for on-disk storage persist beyond the
+ * end of the current transaction.  NOTE: It's the caller's responsibility to
+ * create such a tuplestore in a memory context that will also survive
+ * transaction boundaries, and to ensure the tuplestore is closed when it's
+ * no longer wanted.
  *
  * maxKBytes: how much data to store in memory (any data beyond this
  * amount is paged to disk).
  */
 Tuplestorestate *
-tuplestore_begin_heap(bool randomAccess, bool interTxn, int maxKBytes)
+tuplestore_begin_heap(bool randomAccess, bool interXact, int maxKBytes)
 {
 	Tuplestorestate *state = tuplestore_begin_common(randomAccess,
-													 interTxn, maxKBytes);
+													 interXact,
+													 maxKBytes);
 
 	state->copytup = copytup_heap;
 	state->writetup = writetup_heap;
@@ -341,7 +344,7 @@ tuplestore_puttuple(Tuplestorestate *state, void *tuple)
 			/*
 			 * Nope; time to switch to tape-based operation.
 			 */
-			state->myfile = BufFileCreateTemp(state->interTxn);
+			state->myfile = BufFileCreateTemp(state->interXact);
 			state->status = TSS_WRITEFILE;
 			dumptuples(state);
 			break;

src/include/commands/portalcmds.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: portalcmds.h,v 1.6 2003/03/11 19:40:23 tgl Exp $
+ * $Id: portalcmds.h,v 1.7 2003/04/29 03:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -29,6 +29,6 @@ extern long DoPortalFetch(Portal portal,
 
 extern void PerformPortalClose(char *name);
 
-extern void PortalCleanup(Portal portal);
+extern void PortalCleanup(Portal portal, bool isError);
 
 #endif   /* PORTALCMDS_H */

src/include/storage/buffile.h

@@ -18,7 +18,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: buffile.h,v 1.13 2003/03/27 16:51:29 momjian Exp $
+ * $Id: buffile.h,v 1.14 2003/04/29 03:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -34,7 +34,7 @@ typedef struct BufFile BufFile;
  * prototypes for functions in buffile.c
  */
 
-extern BufFile *BufFileCreateTemp(bool interTxn);
+extern BufFile *BufFileCreateTemp(bool interXact);
 extern void BufFileClose(BufFile *file);
 extern size_t BufFileRead(BufFile *file, void *ptr, size_t size);
 extern size_t BufFileWrite(BufFile *file, void *ptr, size_t size);

src/include/storage/fd.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: fd.h,v 1.37 2003/03/27 16:51:29 momjian Exp $
+ * $Id: fd.h,v 1.38 2003/04/29 03:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -55,7 +55,7 @@ extern int	max_files_per_process;
 /* Operations on virtual Files --- equivalent to Unix kernel file ops */
 extern File FileNameOpenFile(FileName fileName, int fileFlags, int fileMode);
 extern File PathNameOpenFile(FileName fileName, int fileFlags, int fileMode);
-extern File OpenTemporaryFile(bool keepOverTxn);
+extern File OpenTemporaryFile(bool interXact);
 extern void FileClose(File file);
 extern void FileUnlink(File file);
 extern int	FileRead(File file, char *buffer, int amount);

src/include/utils/portal.h

@@ -9,7 +9,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: portal.h,v 1.40 2003/03/27 16:51:29 momjian Exp $
+ * $Id: portal.h,v 1.41 2003/04/29 03:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -48,14 +48,15 @@ typedef struct PortalData *Portal;
 typedef struct PortalData
 {
 	char	   *name;			/* Portal's name */
-	MemoryContext heap;			/* memory for storing short-term data */
+	MemoryContext heap;			/* subsidiary memory */
 	QueryDesc  *queryDesc;		/* Info about query associated with portal */
-	void		(*cleanup) (Portal);	/* Cleanup routine (optional) */
+	void		(*cleanup) (Portal portal, bool isError);	/* Cleanup hook */
 	ScrollType	scrollType;		/* Allow backward fetches? */
-	bool		holdOpen;		/* hold open after txn ends? */
-	TransactionId createXact;	/* the xid of the creating txn */
+	bool		executorRunning;	/* T if we need to call ExecutorEnd */
+	bool		holdOpen;		/* hold open after xact ends? */
+	TransactionId createXact;	/* the xid of the creating xact */
 	Tuplestorestate *holdStore;	/* store for holdable cursors */
-	MemoryContext holdContext;  /* memory for long-term data */
+	MemoryContext holdContext;  /* memory containing holdStore */
 
 	/*
 	 * atStart, atEnd and portalPos indicate the current cursor position.
@@ -88,10 +89,9 @@ typedef struct PortalData
 extern void EnablePortalManager(void);
 extern void AtEOXact_portals(bool isCommit);
 extern Portal CreatePortal(const char *name);
-extern void PortalDrop(Portal portal, bool persistHoldable);
+extern void PortalDrop(Portal portal, bool isError);
 extern Portal GetPortalByName(const char *name);
-extern void PortalSetQuery(Portal portal, QueryDesc *queryDesc,
-						   void (*cleanup) (Portal portal));
+extern void PortalSetQuery(Portal portal, QueryDesc *queryDesc);
 extern void PersistHoldablePortal(Portal portal);
 
 #endif   /* PORTAL_H */

src/include/utils/tuplestore.h

@@ -17,7 +17,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: tuplestore.h,v 1.10 2003/03/27 16:51:29 momjian Exp $
+ * $Id: tuplestore.h,v 1.11 2003/04/29 03:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -37,7 +37,7 @@ typedef struct Tuplestorestate Tuplestorestate;
  */
 
 extern Tuplestorestate *tuplestore_begin_heap(bool randomAccess,
-											  bool interTxn,
+											  bool interXact,
 											  int maxKBytes);
 
 extern void tuplestore_puttuple(Tuplestorestate *state, void *tuple);

src/test/regress/expected/portals.out

@@ -675,7 +675,7 @@ CLOSE foo9;
 CLOSE foo10;
 CLOSE foo11;
 CLOSE foo12;
--- is there a reason why we don't close the rest of the open cursors?
+-- leave some cursors open, to test that auto-close works.
 END;
 --
 -- NO SCROLL disallows backward fetching

src/test/regress/sql/portals.sql

@@ -166,7 +166,7 @@ CLOSE foo11;
 
 CLOSE foo12;
 
--- is there a reason why we don't close the rest of the open cursors?
+-- leave some cursors open, to test that auto-close works.
 
 END;
 
@@ -217,4 +217,4 @@ DECLARE foo26 CURSOR WITH HOLD FOR SELECT * FROM tenk1;
 ROLLBACK;
 
 -- should fail
-FETCH FROM foo26;
+FETCH FROM foo26;
\ No newline at end of file