Revert "Use "transient" files for blind writes, take 2".

tglsfdc · tglsfdc · commit cacb65263b99 · 2012-10-17T12:37:20.000-04:00
This reverts commit fba105b. 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.
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
@@ -1847,10 +1847,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)
@@ -1872,12 +1869,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
@@ -125,11 +125,12 @@ static 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 */
 
-/* Flag to tell whether 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;
 
 typedef struct vfd
 {
@@ -590,7 +591,6 @@ LruDelete(File file)
 	Vfd		   *vfdP;
 
 	Assert(file != 0);
-	Assert(!FileIsNotOpen(file));
 
 	DO_DB(elog(LOG, "LruDelete %d (%s)",
 			   file, VfdCache[file].fileName));
@@ -953,7 +953,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;
@@ -1026,25 +1026,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
  */
@@ -1797,9 +1778,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)
@@ -1822,10 +1802,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
@@ -1842,51 +1819,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
@@ -292,9 +292,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;
@@ -559,9 +556,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;
@@ -1586,9 +1580,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
@@ -165,33 +165,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
@@ -61,7 +61,6 @@ extern int	max_files_per_process;
 /* 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
@@ -62,7 +62,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];
@@ -75,7 +74,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);
