]> granicus.if.org Git - postgresql/commitdiff
Fix bug in temporary file management with subtransactions. A cursor opened
authorHeikki Linnakangas <heikki.linnakangas@iki.fi>
Thu, 3 Dec 2009 11:03:44 +0000 (11:03 +0000)
committerHeikki Linnakangas <heikki.linnakangas@iki.fi>
Thu, 3 Dec 2009 11:03:44 +0000 (11:03 +0000)
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.

src/backend/storage/file/fd.c
src/backend/utils/resowner/resowner.c
src/include/utils/resowner.h

index 2a0108fcee0f00eb0bddd478e5ae9d9673f3208d..eafdde4eb68de4a4db1c827390e4e3cc49a349c9 100644 (file)
@@ -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.143 2008/01/01 19:45:51 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.143.2.1 2009/12/03 11:03:44 heikki Exp $
  *
  * NOTES:
  *
@@ -52,6 +52,7 @@
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "utils/guc.h"
+#include "utils/resowner.h"
 
 
 /*
@@ -125,7 +126,7 @@ typedef struct vfd
 {
        signed short 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;
@@ -831,6 +832,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
        vfdP->fileMode = fileMode;
        vfdP->seekPos = 0;
        vfdP->fdstate = 0x0;
+       vfdP->resowner = NULL;
 
        return file;
 }
@@ -842,11 +844,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)
@@ -884,11 +887,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;
        }
 
        return file;
@@ -1014,6 +1020,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
         */
@@ -1603,24 +1613,6 @@ AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid,
 {
        Index           i;
 
-       if (SizeVfdCache > 0)
-       {
-               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)
@@ -1641,9 +1633,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)
@@ -1691,14 +1684,24 @@ 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);
+                               }
                        }
                }
        }
 
+       /* Clean up "allocated" stdio files and dirs. */
        while (numAllocatedDescs > 0)
                FreeDesc(&allocatedDescs[0]);
 }
index 440aee59c24e0ca4ef0c4a690c9614805827d403..81fa1cca83eb49541a4b6609071d10abd867a5f8 100644 (file)
@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.27 2008/01/01 19:45:55 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.27.2.1 2009/12/03 11:03:44 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -65,6 +65,11 @@ typedef struct ResourceOwnerData
        int                     ntupdescs;              /* number of owned tupdesc references */
        TupleDesc  *tupdescs;           /* dynamically allocated array */
        int                     maxtupdescs;    /* 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;
 
 
@@ -97,6 +102,7 @@ static void ResourceOwnerReleaseInternal(ResourceOwner owner,
 static void PrintRelCacheLeakWarning(Relation rel);
 static void PrintPlanCacheLeakWarning(CachedPlan *plan);
 static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
+static void PrintFileLeakWarning(File file);
 
 
 /*****************************************************************************
@@ -301,6 +307,14 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
                        DecrTupleDescRefCount(owner->tupdescs[owner->ntupdescs - 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();
        }
@@ -331,6 +345,7 @@ ResourceOwnerDelete(ResourceOwner owner)
        Assert(owner->nrelrefs == 0);
        Assert(owner->nplanrefs == 0);
        Assert(owner->ntupdescs == 0);
+       Assert(owner->nfiles == 0);
 
        /*
         * Delete children.  The recursive call will delink the child from me, so
@@ -359,6 +374,8 @@ ResourceOwnerDelete(ResourceOwner owner)
                pfree(owner->planrefs);
        if (owner->tupdescs)
                pfree(owner->tupdescs);
+       if (owner->files)
+               pfree(owner->files);
 
        pfree(owner);
 }
@@ -935,3 +952,87 @@ PrintTupleDescLeakWarning(TupleDesc tupdesc)
                 "TupleDesc reference leak: TupleDesc %p (%u,%d) still referenced",
                 tupdesc, tupdesc->tdtypeid, tupdesc->tdtypmod);
 }
+
+
+/*
+ * 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);
+}
index 1369aa7f683846c2aa6f2a341d5ecbbc8a41e0ec..f804e32d610c2d75ff802e459061a82cefc4faa9 100644 (file)
@@ -12,7 +12,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.15 2008/01/01 19:45:59 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.15.2.1 2009/12/03 11:03:44 heikki Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -20,6 +20,7 @@
 #define RESOWNER_H
 
 #include "storage/buf.h"
+#include "storage/fd.h"
 #include "utils/catcache.h"
 #include "utils/plancache.h"
 
@@ -121,4 +122,11 @@ extern void ResourceOwnerRememberTupleDesc(ResourceOwner owner,
 extern void ResourceOwnerForgetTupleDesc(ResourceOwner owner,
                                                         TupleDesc tupdesc);
 
+/* 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 */