From b9c44be65ffa9b26c72e548292ac81c3f17cf7f7 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 3 Dec 2009 11:04:03 +0000 Subject: [PATCH] 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. --- src/backend/storage/file/fd.c | 65 ++++++++-------- src/backend/utils/resowner/resowner.c | 103 +++++++++++++++++++++++++- src/include/utils/resowner.h | 10 ++- 3 files changed, 145 insertions(+), 33 deletions(-) diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index e4ebb93fc92..3e3932b861e 100644 --- a/src/backend/storage/file/fd.c +++ b/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.121.2.3 2006/01/17 23:52:50 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.121.2.4 2009/12/03 11:04:02 heikki Exp $ * * NOTES: * @@ -50,6 +50,7 @@ #include "access/xact.h" #include "storage/fd.h" #include "storage/ipc.h" +#include "utils/resowner.h" /* @@ -123,7 +124,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; @@ -839,6 +840,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode) vfdP->fileMode = fileMode; vfdP->seekPos = 0; vfdP->fdstate = 0x0; + vfdP->resowner = NULL; return file; } @@ -868,11 +870,12 @@ FileNameOpenFile(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) @@ -922,11 +925,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; @@ -973,6 +979,10 @@ FileClose(File file) vfdP->fileName); } + /* Unregister it from the resource owner */ + if (vfdP->resowner) + ResourceOwnerForgetFile(vfdP->resowner, file); + /* * Return the Vfd slot to the free list */ @@ -1515,24 +1525,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) @@ -1553,8 +1545,9 @@ 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. + * 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. */ void AtEOXact_Files(void) @@ -1600,14 +1593,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]); } diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index fb01b1a5fba..573d1d4b459 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -14,7 +14,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.14.2.2 2005/12/08 19:19:31 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/utils/resowner/resowner.c,v 1.14.2.3 2009/12/03 11:04:03 heikki Exp $ * *------------------------------------------------------------------------- */ @@ -58,6 +58,11 @@ typedef struct ResourceOwnerData int nrelrefs; /* number of owned relcache pins */ Relation *relrefs; /* dynamically allocated array */ int maxrelrefs; /* 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; @@ -88,6 +93,7 @@ static void ResourceOwnerReleaseInternal(ResourceOwner owner, bool isCommit, bool isTopLevel); static void PrintRelCacheLeakWarning(Relation rel); +static void PrintFileLeakWarning(File file); /***************************************************************************** @@ -277,6 +283,14 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, ReleaseCatCacheList(owner->catlistrefs[owner->ncatlistrefs - 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_gist(); ReleaseResources_hash(); @@ -307,6 +321,7 @@ ResourceOwnerDelete(ResourceOwner owner) Assert(owner->ncatrefs == 0); Assert(owner->ncatlistrefs == 0); Assert(owner->nrelrefs == 0); + Assert(owner->nfiles == 0); /* * Delete children. The recursive call will delink the child from me, so @@ -331,6 +346,8 @@ ResourceOwnerDelete(ResourceOwner owner) pfree(owner->catlistrefs); if (owner->relrefs) pfree(owner->relrefs); + if (owner->files) + pfree(owner->files); pfree(owner); } @@ -745,3 +762,87 @@ PrintRelCacheLeakWarning(Relation rel) elog(WARNING, "relcache reference leak: relation \"%s\" not closed", RelationGetRelationName(rel)); } + + +/* + * 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); +} diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h index 11e6f4eee2d..28d1ca51627 100644 --- a/src/include/utils/resowner.h +++ b/src/include/utils/resowner.h @@ -12,7 +12,7 @@ * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.5 2004/12/31 22:03:46 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/utils/resowner.h,v 1.5.6.1 2009/12/03 11:04:03 heikki Exp $ * *------------------------------------------------------------------------- */ @@ -20,6 +20,7 @@ #define RESOWNER_H #include "storage/buf.h" +#include "storage/fd.h" #include "utils/catcache.h" #include "utils/rel.h" @@ -107,4 +108,11 @@ extern void ResourceOwnerRememberRelationRef(ResourceOwner owner, extern void ResourceOwnerForgetRelationRef(ResourceOwner owner, Relation rel); +/* 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 */