From 9bacf0e3734f0b5dc821abf7cb312377876e22d3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 17 Oct 2012 12:37:08 -0400 Subject: [PATCH] Revert "Use "transient" files for blind writes, take 2". This reverts commit fba105b1099f4f5fa7283bb17cba6fed2baa8d0c. That approach had problems with the smgr-level state not tracking what we really want to happen, and with the VFD-level state not tracking the smgr-level state very well either. In consequence, it was still possible to hold kernel file descriptors open for long-gone tables (as in recent report from Tore Halset), and yet there were also cases of FDs being closed undesirably soon. A replacement implementation will follow. --- src/backend/storage/buffer/bufmgr.c | 10 +--- src/backend/storage/file/fd.c | 91 +++++++++-------------------- src/backend/storage/smgr/md.c | 9 --- src/backend/storage/smgr/smgr.c | 17 ------ src/include/storage/fd.h | 1 - src/include/storage/smgr.h | 2 - 6 files changed, 28 insertions(+), 102 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 56095b3250..bdcbe47ac9 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1882,10 +1882,7 @@ BufferGetTag(Buffer buffer, RelFileNode *rnode, ForkNumber *forknum, * written.) * * If the caller has an smgr reference for the buffer's relation, pass it - * as the second parameter. If not, pass NULL. In the latter case, the - * relation will be marked as "transient" so that the corresponding - * kernel-level file descriptors are closed when the current transaction ends, - * if any. + * as the second parameter. If not, pass NULL. */ static void FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln) @@ -1909,12 +1906,9 @@ FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln) errcontext.previous = error_context_stack; error_context_stack = &errcontext; - /* Find smgr relation for buffer, and mark it as transient */ + /* Find smgr relation for buffer */ if (reln == NULL) - { reln = smgropen(buf->tag.rnode, InvalidBackendId); - smgrsettransient(reln); - } TRACE_POSTGRESQL_BUFFER_FLUSH_START(buf->tag.forkNum, buf->tag.blockNum, diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index fed25fd7e7..ecb62ba01a 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -126,8 +126,6 @@ int max_safe_fds = 32; /* default if not changed */ /* 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 */ -#define FD_XACT_TRANSIENT (1 << 2) /* T = close (not delete) at aoXact, - * but keep VFD */ typedef struct vfd { @@ -158,8 +156,11 @@ static Size SizeVfdCache = 0; */ static int nfile = 0; -/* True if there are files to close/delete at end of transaction */ -static bool have_pending_fd_cleanup = false; +/* + * Flag to tell whether it's worth scanning VfdCache looking for temp files + * to close + */ +static bool have_xact_temporary_files = false; /* * Tracks the total size of all temporary files. Note: when temp_file_limit @@ -607,7 +608,6 @@ LruDelete(File file) Vfd *vfdP; Assert(file != 0); - Assert(!FileIsNotOpen(file)); DO_DB(elog(LOG, "LruDelete %d (%s)", file, VfdCache[file].fileName)); @@ -971,7 +971,7 @@ OpenTemporaryFile(bool interXact) VfdCache[file].resowner = CurrentResourceOwner; /* ensure cleanup happens at eoxact */ - have_pending_fd_cleanup = true; + have_xact_temporary_files = true; } return file; @@ -1044,25 +1044,6 @@ OpenTemporaryFileInTablespace(Oid tblspcOid, bool rejectError) return file; } -/* - * Set the transient flag on a file - * - * This flag tells CleanupTempFiles to close the kernel-level file descriptor - * (but not the VFD itself) at end of transaction. - */ -void -FileSetTransient(File file) -{ - Vfd *vfdP; - - Assert(FileIsValid(file)); - - vfdP = &VfdCache[file]; - vfdP->fdstate |= FD_XACT_TRANSIENT; - - have_pending_fd_cleanup = true; -} - /* * close a file when done with it */ @@ -1863,9 +1844,8 @@ AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid, * particularly care which). All still-open per-transaction temporary file * VFDs are closed, which also causes the underlying files to be deleted * (although they should've been closed already by the ResourceOwner - * cleanup). Transient files have their kernel file descriptors closed. - * Furthermore, all "allocated" stdio files are closed. We also forget any - * transaction-local temp tablespace list. + * cleanup). Furthermore, all "allocated" stdio files are closed. We also + * forget any transaction-local temp tablespace list. */ void AtEOXact_Files(void) @@ -1888,10 +1868,7 @@ AtProcExit_Files(int code, Datum arg) } /* - * General cleanup routine for fd.c. - * - * Temporary files are closed, and their underlying files deleted. - * Transient files are closed. + * 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 @@ -1908,51 +1885,35 @@ CleanupTempFiles(bool isProcExit) * Careful here: at proc_exit we need extra cleanup, not just * xact_temporary files. */ - if (isProcExit || have_pending_fd_cleanup) + 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 (VfdCache[i].fileName != NULL) + if ((fdstate & FD_TEMPORARY) && VfdCache[i].fileName != NULL) { - if (fdstate & FD_TEMPORARY) - { - /* - * 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. They - * should be closed by the ResourceOwner mechanism - * already, so this is just a debugging cross-check. - */ - 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); - } - } - else if (fdstate & FD_XACT_TRANSIENT) + /* + * 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. They should be closed by + * the ResourceOwner mechanism already, so this is just a + * debugging cross-check. + */ + if (isProcExit) + FileClose(i); + else if (fdstate & FD_XACT_TEMPORARY) { - /* - * Close the FD, and remove the entry from the LRU ring, - * but also remove the flag from the VFD. This is to - * ensure that if the VFD is reused in the future for - * non-transient access, we don't close it inappropriately - * then. - */ - if (!FileIsNotOpen(i)) - LruDelete(i); - VfdCache[i].fdstate &= ~FD_XACT_TRANSIENT; + elog(WARNING, + "temporary file %s not closed at end-of-transaction", + VfdCache[i].fileName); + FileClose(i); } } } - have_pending_fd_cleanup = false; + have_xact_temporary_files = false; } /* Clean up "allocated" stdio files and dirs. */ diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 97742b92bb..3f4ab49dab 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -300,9 +300,6 @@ mdcreate(SMgrRelation reln, ForkNumber forkNum, bool isRedo) pfree(path); - if (reln->smgr_transient) - FileSetTransient(fd); - reln->md_fd[forkNum] = _fdvec_alloc(); reln->md_fd[forkNum]->mdfd_vfd = fd; @@ -585,9 +582,6 @@ mdopen(SMgrRelation reln, ForkNumber forknum, ExtensionBehavior behavior) pfree(path); - if (reln->smgr_transient) - FileSetTransient(fd); - reln->md_fd[forknum] = mdfd = _fdvec_alloc(); mdfd->mdfd_vfd = fd; @@ -1680,9 +1674,6 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno, if (fd < 0) return NULL; - if (reln->smgr_transient) - FileSetTransient(fd); - /* allocate an mdfdvec entry for it */ v = _fdvec_alloc(); diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index 0cec1477f3..6319d1e858 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -163,33 +163,16 @@ smgropen(RelFileNode rnode, BackendId backend) reln->smgr_targblock = InvalidBlockNumber; reln->smgr_fsm_nblocks = InvalidBlockNumber; reln->smgr_vm_nblocks = InvalidBlockNumber; - reln->smgr_transient = false; reln->smgr_which = 0; /* we only have md.c at present */ /* mark it not open */ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) reln->md_fd[forknum] = NULL; } - else - /* if it was transient before, it no longer is */ - reln->smgr_transient = false; return reln; } -/* - * smgrsettransient() -- mark an SMgrRelation object as transaction-bound - * - * The main effect of this is that all opened files are marked to be - * kernel-level closed (but not necessarily VFD-closed) when the current - * transaction ends. - */ -void -smgrsettransient(SMgrRelation reln) -{ - reln->smgr_transient = true; -} - /* * smgrsetowner() -- Establish a long-lived reference to an SMgrRelation object * diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index bad9f10c62..849bb1025d 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -66,7 +66,6 @@ extern int max_safe_fds; /* Operations on virtual Files --- equivalent to Unix kernel file ops */ extern File PathNameOpenFile(FileName fileName, int fileFlags, int fileMode); extern File OpenTemporaryFile(bool interXact); -extern void FileSetTransient(File file); extern void FileClose(File file); extern int FilePrefetch(File file, off_t offset, int amount); extern int FileRead(File file, char *buffer, int amount); diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h index 3560d53907..92b9198d54 100644 --- a/src/include/storage/smgr.h +++ b/src/include/storage/smgr.h @@ -60,7 +60,6 @@ typedef struct SMgrRelationData * submodules. Do not touch them from elsewhere. */ int smgr_which; /* storage manager selector */ - bool smgr_transient; /* T if files are to be closed at EOXact */ /* for md.c; NULL for forks that are not open */ struct _MdfdVec *md_fd[MAX_FORKNUM + 1]; @@ -73,7 +72,6 @@ typedef SMgrRelationData *SMgrRelation; extern void smgrinit(void); extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend); -extern void smgrsettransient(SMgrRelation reln); extern bool smgrexists(SMgrRelation reln, ForkNumber forknum); extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln); extern void smgrclose(SMgrRelation reln); -- 2.40.0