Fix bug in temporary file management with subtransactions. A cursor opened

in a subtransaction stays open even if the subtransaction is aborted, so
any temporary files related to it must stay alive as well. With the patch,
we use ResourceOwners to track open temporary files and don't automatically
close them at subtransaction end (though in the normal case temporary files
are registered with the subtransaction resource owner and will therefore be
closed).

At end of top transaction, we still check that there's no temporary files
marked as close-at-end-of-transaction open, but that's now just a debugging
cross-check as the resource owner cleanup should've closed them already.

Author: hlinnaka

src/backend/storage/file/fd.c

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.150 2009/08/05 18:01:54 heikki Exp $
+ *	  $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.151 2009/12/03 11:03:28 heikki Exp $
  *
  * NOTES:
  *
@@ -55,6 +55,7 @@
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "utils/guc.h"
+#include "utils/resowner.h"
 
 
 /*
@@ -134,7 +135,7 @@ typedef struct vfd
 {
 	int			fd;				/* current FD, or VFD_CLOSED if none */
 	unsigned short fdstate;		/* bitflags for VFD's state */
-	SubTransactionId create_subid;		/* for TEMPORARY fds, creating subxact */
+	ResourceOwner resowner;		/* owner, for automatic cleanup */
 	File		nextFree;		/* link to next free VFD, if in freelist */
 	File		lruMoreRecently;	/* doubly linked recency-of-use list */
 	File		lruLessRecently;
@@ -865,6 +866,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
 	vfdP->fileMode = fileMode;
 	vfdP->seekPos = 0;
 	vfdP->fdstate = 0x0;
+	vfdP->resowner = NULL;
 
 	return file;
 }
@@ -876,11 +878,12 @@ 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.
  *
- * 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 explicitly closed.
+ * Unless interXact is true, the file is remembered by CurrentResourceOwner
+ * to ensure it's closed and deleted when it's no longer needed, typically at
+ * the 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 explicitly closed.
  */
 File
 OpenTemporaryFile(bool interXact)
@@ -918,11 +921,14 @@ OpenTemporaryFile(bool interXact)
 	/* Mark it for deletion at close */
 	VfdCache[file].fdstate |= FD_TEMPORARY;
 
-	/* Mark it for deletion at EOXact */
+	/* Register it with the current resource owner */
 	if (!interXact)
 	{
 		VfdCache[file].fdstate |= FD_XACT_TEMPORARY;
-		VfdCache[file].create_subid = GetCurrentSubTransactionId();
+
+		ResourceOwnerEnlargeFiles(CurrentResourceOwner);
+		ResourceOwnerRememberFile(CurrentResourceOwner, file);
+		VfdCache[file].resowner = CurrentResourceOwner;
 
 		/* ensure cleanup happens at eoxact */
 		have_xact_temporary_files = true;
@@ -1051,6 +1057,10 @@ FileClose(File file)
 			elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
 	}
 
+	/* Unregister it from the resource owner */
+	if (vfdP->resowner)
+		ResourceOwnerForgetFile(vfdP->resowner, file);
+
 	/*
 	 * Return the Vfd slot to the free list
 	 */
@@ -1695,24 +1705,6 @@ AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid,
 {
 	Index		i;
 
-	if (have_xact_temporary_files)
-	{
-		Assert(FileIsNotOpen(0));		/* Make sure ring not corrupted */
-		for (i = 1; i < SizeVfdCache; i++)
-		{
-			unsigned short fdstate = VfdCache[i].fdstate;
-
-			if ((fdstate & FD_XACT_TEMPORARY) &&
-				VfdCache[i].create_subid == mySubid)
-			{
-				if (isCommit)
-					VfdCache[i].create_subid = parentSubid;
-				else if (VfdCache[i].fileName != NULL)
-					FileClose(i);
-			}
-		}
-	}
-
 	for (i = 0; i < numAllocatedDescs; i++)
 	{
 		if (allocatedDescs[i].create_subid == mySubid)
@@ -1733,9 +1725,10 @@ AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid,
  *
  * 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.
- * We also forget any transaction-local temp tablespace list.
+ * VFDs are closed, which also causes the underlying files to be deleted
+ * (although they should've been closed already by the ResourceOwner
+ * cleanup). Furthermore, all "allocated" stdio files are closed. We also
+ * forget any transaction-local temp tablespace list.
  */
 void
 AtEOXact_Files(void)
@@ -1787,16 +1780,26 @@ CleanupTempFiles(bool isProcExit)
 				/*
 				 * 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.
+				 * local to the current transaction. They should be closed
+				 * by the ResourceOwner mechanism already, so this is just
+				 * a debugging cross-check.
 				 */
-				if (isProcExit || (fdstate & FD_XACT_TEMPORARY))
+				if (isProcExit)
+					FileClose(i);
+				else if (fdstate & FD_XACT_TEMPORARY)
+				{
+					elog(WARNING,
+						 "temporary file %s not closed at end-of-transaction",
+						 VfdCache[i].fileName);
 					FileClose(i);
+				}
 			}
 		}
 
 		have_xact_temporary_files = false;
 	}
 
+	/* Clean up "allocated" stdio files and dirs. */
 	while (numAllocatedDescs > 0)
 		FreeDesc(&allocatedDescs[0]);
 }

src/backend/utils/resowner/resowner.c

@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.32 2009/06/11 14:49:06 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.33 2009/12/03 11:03:29 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -72,6 +72,11 @@ typedef struct ResourceOwnerData
 	int			nsnapshots;		/* number of owned snapshot references */
 	Snapshot   *snapshots;		/* dynamically allocated array */
 	int			maxsnapshots;	/* currently allocated array size */
+
+	/* We have built-in support for remembering open temporary files */
+	int			nfiles;			/* number of owned temporary files */
+	File	   *files;			/* dynamically allocated array */
+	int			maxfiles;		/* currently allocated array size */
 } ResourceOwnerData;
 
 
@@ -105,6 +110,7 @@ static void PrintRelCacheLeakWarning(Relation rel);
 static void PrintPlanCacheLeakWarning(CachedPlan *plan);
 static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
 static void PrintSnapshotLeakWarning(Snapshot snapshot);
+static void PrintFileLeakWarning(File file);
 
 
 /*****************************************************************************
@@ -316,6 +322,14 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
 			UnregisterSnapshot(owner->snapshots[owner->nsnapshots - 1]);
 		}
 
+		/* Ditto for temporary files */
+		while (owner->nfiles > 0)
+		{
+			if (isCommit)
+				PrintFileLeakWarning(owner->files[owner->nfiles - 1]);
+			FileClose(owner->files[owner->nfiles - 1]);
+		}
+
 		/* Clean up index scans too */
 		ReleaseResources_hash();
 	}
@@ -347,6 +361,7 @@ ResourceOwnerDelete(ResourceOwner owner)
 	Assert(owner->nplanrefs == 0);
 	Assert(owner->ntupdescs == 0);
 	Assert(owner->nsnapshots == 0);
+	Assert(owner->nfiles == 0);
 
 	/*
 	 * Delete children.  The recursive call will delink the child from me, so
@@ -377,6 +392,8 @@ ResourceOwnerDelete(ResourceOwner owner)
 		pfree(owner->tupdescs);
 	if (owner->snapshots)
 		pfree(owner->snapshots);
+	if (owner->files)
+		pfree(owner->files);
 
 	pfree(owner);
 }
@@ -1035,3 +1052,87 @@ PrintSnapshotLeakWarning(Snapshot snapshot)
 		 "Snapshot reference leak: Snapshot %p still referenced",
 		 snapshot);
 }
+
+
+/*
+ * Make sure there is room for at least one more entry in a ResourceOwner's
+ * files reference array.
+ *
+ * This is separate from actually inserting an entry because if we run out
+ * of memory, it's critical to do so *before* acquiring the resource.
+ */
+void
+ResourceOwnerEnlargeFiles(ResourceOwner owner)
+{
+	int			newmax;
+
+	if (owner->nfiles < owner->maxfiles)
+		return;					/* nothing to do */
+
+	if (owner->files == NULL)
+	{
+		newmax = 16;
+		owner->files = (File *)
+			MemoryContextAlloc(TopMemoryContext, newmax * sizeof(File));
+		owner->maxfiles = newmax;
+	}
+	else
+	{
+		newmax = owner->maxfiles * 2;
+		owner->files = (File *)
+			repalloc(owner->files, newmax * sizeof(File));
+		owner->maxfiles = newmax;
+	}
+}
+
+/*
+ * Remember that a temporary file is owned by a ResourceOwner
+ *
+ * Caller must have previously done ResourceOwnerEnlargeFiles()
+ */
+void
+ResourceOwnerRememberFile(ResourceOwner owner, File file)
+{
+	Assert(owner->nfiles < owner->maxfiles);
+	owner->files[owner->nfiles] = file;
+	owner->nfiles++;
+}
+
+/*
+ * Forget that a temporary file is owned by a ResourceOwner
+ */
+void
+ResourceOwnerForgetFile(ResourceOwner owner, File file)
+{
+	File	   *files = owner->files;
+	int			ns1 = owner->nfiles - 1;
+	int			i;
+
+	for (i = ns1; i >= 0; i--)
+	{
+		if (files[i] == file)
+		{
+			while (i < ns1)
+			{
+				files[i] = files[i + 1];
+				i++;
+			}
+			owner->nfiles = ns1;
+			return;
+		}
+	}
+	elog(ERROR, "temporery file %d is not owned by resource owner %s",
+		 file, owner->name);
+}
+
+
+/*
+ * Debugging subroutine
+ */
+static void
+PrintFileLeakWarning(File file)
+{
+	elog(WARNING,
+		 "temporary file leak: File %d still referenced",
+		 file);
+}

src/include/utils/resowner.h

@@ -12,14 +12,15 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.17 2009/01/01 17:24:02 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.18 2009/12/03 11:03:29 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
 #ifndef RESOWNER_H
 #define RESOWNER_H
 
 #include "storage/buf.h"
+#include "storage/fd.h"
 #include "utils/catcache.h"
 #include "utils/plancache.h"
 #include "utils/snapshot.h"
@@ -129,4 +130,11 @@ extern void ResourceOwnerRememberSnapshot(ResourceOwner owner,
 extern void ResourceOwnerForgetSnapshot(ResourceOwner owner,
 							Snapshot snapshot);
 
+/* support for temporary file management */
+extern void ResourceOwnerEnlargeFiles(ResourceOwner owner);
+extern void ResourceOwnerRememberFile(ResourceOwner owner,
+						  File file);
+extern void ResourceOwnerForgetFile(ResourceOwner owner,
+						File file);
+
 #endif   /* RESOWNER_H */