Fix error handling in temp-file deletion with log_temp_files active.

tglsfdc · tglsfdc · commit c6c67505e4b7 · 2010-11-08T22:14:55.000-05:00
The original coding in FileClose() reset the file-is-temp flag before
unlinking the file, so that if control came back through due to an error,
it wouldn't try to unlink the file twice.  This was correct when written,
but when the log_temp_files feature was added, the logging action was put
in between those two steps.  An error occurring during the logging action
--- such as a query cancel --- would result in the unlink not getting done
at all, as in recent report from Michael Glaesemann.

To fix this, make sure that we do both the stat and the unlink before doing
anything that could conceivably CHECK_FOR_INTERRUPTS.  There is a judgment
call here, which is which log message to emit first: if you can see only
one, which should it be?  I chose to log unlink failure at the risk of
losing the log_temp_files log message --- after all, if the unlink does
fail, the temp file is still there for you to see.

Back-patch to all versions that have log_temp_files.  The code was OK
before that.
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
@@ -1029,7 +1029,6 @@ void
 FileClose(File file)
 {
 	Vfd		   *vfdP;
-	struct stat filestats;
 
 	Assert(FileIsValid(file));
 
@@ -1052,15 +1051,36 @@ FileClose(File file)
 	}
 
 	/*
-	 * Delete the file if it was temporary
+	 * Delete the file if it was temporary, and make a log entry if wanted
 	 */
 	if (vfdP->fdstate & FD_TEMPORARY)
 	{
-		/* reset flag so that die() interrupt won't cause problems */
+		/*
+		 * If we get an error, as could happen within the ereport/elog calls,
+		 * we'll come right back here during transaction abort.  Reset the
+		 * flag to ensure that we can't get into an infinite loop.  This code
+		 * is arranged to ensure that the worst-case consequence is failing
+		 * to emit log message(s), not failing to attempt the unlink.
+		 */
 		vfdP->fdstate &= ~FD_TEMPORARY;
+
 		if (log_temp_files >= 0)
 		{
-			if (stat(vfdP->fileName, &filestats) == 0)
+			struct stat filestats;
+			int		stat_errno;
+
+			/* first try the stat() */
+			if (stat(vfdP->fileName, &filestats))
+				stat_errno = errno;
+			else
+				stat_errno = 0;
+
+			/* in any case do the unlink */
+			if (unlink(vfdP->fileName))
+				elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
+
+			/* and last report the stat results */
+			if (stat_errno == 0)
 			{
 				if ((filestats.st_size / 1024) >= log_temp_files)
 					ereport(LOG,
@@ -1069,10 +1089,17 @@ FileClose(File file)
 									(unsigned long) filestats.st_size)));
 			}
 			else
+			{
+				errno = stat_errno;
 				elog(LOG, "could not stat file \"%s\": %m", vfdP->fileName);
+			}
+		}
+		else
+		{
+			/* easy case, just do the unlink */
+			if (unlink(vfdP->fileName))
+				elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
 		}
-		if (unlink(vfdP->fileName))
-			elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
 	}
 
 	/* Unregister it from the resource owner */
