Re: Assertion failure with a subtransaction and cursor

Heikki Linnakangas wrote:
> Tom Lane wrote:
>> So as far as I can tell at the moment, temp files really are the only
>> problem, and making them be managed by resource owners instead of a
>> subxact-based release policy should fix that.
>
> Ok, good.
>
>> I can go work on that, unless you wanted to?
>
> I started hacking on that when I posted, so I can finish it.

So, here's what I got this far, which is pretty straightforward.
Temporary files not opened in the interXact mode are registered with
CurrentResourceOwner, ensuring they're cleaned up at at the end of
(sub)transaction, or when the portal is closed if it's a temporary file
related to a cursor.

The logical next step would be to get rid of the interXact concept
altogether and always associate temporary files with the current
resource owner; the caller should switch to a sufficiently long-lived
one before calling. But I'm hesitant to change the signature of
OpenTemporaryFile, and tuplestore_begin_heap doesn't need the interXact
argument anymore either. Changing the signature of tuplestore_begin_heap
would certainly break user-defined set-returning C functions. So at
least in back-branches, perhaps we should leave those as dummy arguments
and elog(ERROR) if interXact is not passed in as false.

Google turned up one extra module that calls tuplestore_begin_heap with
interXact set to 'true', but frankly that one looks broken to me. It
should be using 'false':
http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/python/be/src/call/pl.c?rev=1.20&content-type=text/x-cvsweb-markup

I left the cleanup of files/dirs opened with AllocateFile/Dir alone,
although it looks a bit inconsistent now that they don't use resource
owners as well.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.150
diff -c -r1.150 fd.c
*** src/backend/storage/file/fd.c    5 Aug 2009 18:01:54 -0000    1.150
--- src/backend/storage/file/fd.c    2 Dec 2009 21:42:01 -0000
***************
*** 55,60 ****
--- 55,61 ----
  #include "storage/fd.h"
  #include "storage/ipc.h"
  #include "utils/guc.h"
+ #include "utils/resowner.h"


  /*
***************
*** 122,140 ****

  /* these are the assigned bits in fdstate below: */
  #define FD_TEMPORARY        (1 << 0)    /* T = delete when closed */
- #define FD_XACT_TEMPORARY    (1 << 1)    /* T = delete at eoXact */
-
- /*
-  * Flag to tell whether it's worth scanning VfdCache looking for temp files to
-  * close
-  */
- static bool have_xact_temporary_files = false;

  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 */
      File        nextFree;        /* link to next free VFD, if in freelist */
      File        lruMoreRecently;    /* doubly linked recency-of-use list */
      File        lruLessRecently;
--- 123,134 ----

  /* these are the assigned bits in fdstate below: */
  #define FD_TEMPORARY        (1 << 0)    /* T = delete when closed */

  typedef struct vfd
  {
      int            fd;                /* current FD, or VFD_CLOSED if none */
      unsigned short fdstate;        /* bitflags for VFD's state */
!     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,870 ****
--- 859,865 ----
      vfdP->fileMode = fileMode;
      vfdP->seekPos = 0;
      vfdP->fdstate = 0x0;
+     vfdP->resowner = NULL;

      return file;
  }
***************
*** 876,883 ****
   * 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.
--- 871,878 ----
   * 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 associate the file with CurrentResourceOwner.
!  * 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.
***************
*** 918,931 ****
      /* Mark it for deletion at close */
      VfdCache[file].fdstate |= FD_TEMPORARY;

!     /* Mark it for deletion at EOXact */
      if (!interXact)
      {
!         VfdCache[file].fdstate |= FD_XACT_TEMPORARY;
!         VfdCache[file].create_subid = GetCurrentSubTransactionId();
!
!         /* ensure cleanup happens at eoxact */
!         have_xact_temporary_files = true;
      }

      return file;
--- 913,924 ----
      /* Mark it for deletion at close */
      VfdCache[file].fdstate |= FD_TEMPORARY;

!     /* Associate it with the current resource owner */
      if (!interXact)
      {
!         ResourceOwnerEnlargeFiles(CurrentResourceOwner);
!         ResourceOwnerRememberFile(CurrentResourceOwner, file);
!         VfdCache[file].resowner = CurrentResourceOwner;
      }

      return file;
***************
*** 1051,1056 ****
--- 1044,1052 ----
              elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
      }

+     if (vfdP->resowner)
+         ResourceOwnerForgetFile(vfdP->resowner, file);
+
      /*
       * Return the Vfd slot to the free list
       */
***************
*** 1695,1718 ****
  {
      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)
--- 1691,1696 ----
***************
*** 1732,1746 ****
   * AtEOXact_Files
   *
   * 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.
   */
  void
  AtEOXact_Files(void)
  {
!     CleanupTempFiles(false);
      tempTableSpaces = NULL;
      numTempTableSpaces = -1;
  }
--- 1710,1725 ----
   * AtEOXact_Files
   *
   * This routine is called during transaction commit or abort (it doesn't
!  * particularly care which).  All "allocated" stdio files are closed.
   * We also forget any transaction-local temp tablespace list.
   */
  void
  AtEOXact_Files(void)
  {
!     /* Clean up "allocated" stdio files and dirs. */
!     while (numAllocatedDescs > 0)
!         FreeDesc(&allocatedDescs[0]);
!
      tempTableSpaces = NULL;
      numTempTableSpaces = -1;
  }
***************
*** 1749,1802 ****
   * 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(int code, Datum arg)
  {
!     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 and dirs.
!  */
! static void
! CleanupTempFiles(bool isProcExit)
! {
!     Index        i;

      /*
!      * Careful here: at proc_exit we need extra cleanup, not just
!      * xact_temporary files.
       */
!     if (isProcExit || 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_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);
!             }
!         }

!         have_xact_temporary_files = false;
      }

      while (numAllocatedDescs > 0)
          FreeDesc(&allocatedDescs[0]);
  }
--- 1728,1755 ----
   * AtProcExit_Files
   *
   * on_proc_exit hook to clean up temp files during backend shutdown.
!  * Here, we want to clean up *all* temp files and "allocated" stdio files
!  * and dirs.
   */
  static void
  AtProcExit_Files(int code, Datum arg)
  {
!     int i;

      /*
!      * Close all still-open temporary file VFD, which also causes the
!      * underlying files to be deleted.
       */
!     Assert(FileIsNotOpen(0));        /* Make sure ring not corrupted */
!     for (i = 1; i < SizeVfdCache; i++)
      {
!         unsigned short fdstate = VfdCache[i].fdstate;

!         if ((fdstate & FD_TEMPORARY) && VfdCache[i].fileName != NULL)
!             FileClose(i);
      }

+     /* Clean up "allocated" stdio files and dirs. */
      while (numAllocatedDescs > 0)
          FreeDesc(&allocatedDescs[0]);
  }
Index: src/backend/utils/resowner/resowner.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/resowner/resowner.c,v
retrieving revision 1.32
diff -c -r1.32 resowner.c
*** src/backend/utils/resowner/resowner.c    11 Jun 2009 14:49:06 -0000    1.32
--- src/backend/utils/resowner/resowner.c    2 Dec 2009 21:42:01 -0000
***************
*** 72,77 ****
--- 72,82 ----
      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,110 ****
--- 110,116 ----
  static void PrintPlanCacheLeakWarning(CachedPlan *plan);
  static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
  static void PrintSnapshotLeakWarning(Snapshot snapshot);
+ static void PrintFileLeakWarning(File file);


  /*****************************************************************************
***************
*** 316,321 ****
--- 322,335 ----
              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,352 ****
--- 361,367 ----
      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,382 ****
--- 392,399 ----
          pfree(owner->tupdescs);
      if (owner->snapshots)
          pfree(owner->snapshots);
+     if (owner->files)
+         pfree(owner->files);

      pfree(owner);
  }
***************
*** 1035,1037 ****
--- 1052,1138 ----
           "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);
+ }
Index: src/include/utils/resowner.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/resowner.h,v
retrieving revision 1.17
diff -c -r1.17 resowner.h
*** src/include/utils/resowner.h    1 Jan 2009 17:24:02 -0000    1.17
--- src/include/utils/resowner.h    2 Dec 2009 21:42:01 -0000
***************
*** 20,25 ****
--- 20,26 ----
  #define RESOWNER_H

  #include "storage/buf.h"
+ #include "storage/fd.h"
  #include "utils/catcache.h"
  #include "utils/plancache.h"
  #include "utils/snapshot.h"
***************
*** 129,132 ****
--- 130,140 ----
  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 */