8000 Fix error handling in temp-file deletion with log_temp_files active. · justtesting112233/postgres@658a630 · GitHub
[go: up one dir, main page]
Skip to content

Commit 658a630

Browse files
tglsfdctglsfdc
committed
Fix error handling in temp-file deletion with log_temp_files active.
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.
1 parent faa9007 commit 658a630

File tree

1 file changed

+33
-6
lines changed
  • src/backend/storage/file

1 file changed

+33
-6
lines changed

‎src/backend/storage/file/fd.c

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,7 +1011,6 @@ void
10111011
FileClose(File file)
10121012
{
10131013
Vfd *vfdP;
1014-
struct stat filestats;
10151014

10161015
Assert(FileIsValid(file));
10171016

@@ -1034,15 +1033,36 @@ FileClose(File file)
10341033
}
10351034

10361035
/*
1037-
* Delete the file if it was temporary
1036+
* Delete the file if it was temporary, and make a log entry if wanted
10381037
*/
10391038
if (vfdP->fdstate & FD_TEMPORARY)
10401039
{
1041-
/* reset flag so that die() interrupt won't cause problems */
1040+
/*
1041+
* If we get an error, as could happen within the ereport/elog calls,
1042+
* we'll come right back here during transaction abort. Reset the
1043+
* flag to ensure that we can't get into an infinite loop. This code
1044+
* is arranged to ensure that the worst-case consequence is failing
1045+
* to emit log message(s), not failing to attempt the unlink.
1046+
*/
10421047
vfdP->fdstate &= ~FD_TEMPORARY;
1048+
10431049
if (log_temp_files >= 0)
10441050
{
1045-
if (stat(vfdP->fileName, &filestats) == 0)
1051+
struct stat filestats;
1052+
int stat_errno;
1053+
1054+
/* first try the stat() */
1055+
if (stat(vfdP->fileName, &filestats))
1056+
stat_errno = errno;
1057+
else
1058+
stat_errno = 0;
1059+
1060+
/* in any case do the unlink */
1061+
if (unlink(vfdP->fileName))
1062+
elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
1063+
1064+
/* and last report the stat results */
1065+
if (stat_errno == 0)
10461066
{
10471067
if (filestats.st_size >= log_temp_files)
10481068
ereport(LOG,
@@ -1051,10 +1071,17 @@ FileClose(File file)
10511071
(unsigned long) filestats.st_size)));
10521072
}
10531073
else
1074+
{
1075+
errno = stat_errno;
10541076
elog(LOG, "could not stat file \"%s\": %m", vfdP->fileName);
1077+
}
1078+
}
1079+
else
1080+
{
1081+
/* easy case, just do the unlink */
1082+
if (unlink(vfdP->fileName))
1083+
elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
10551084
}
1056-
if (unlink(vfdP->fileName))
1057-
elog(LOG, "could not unlink file \"%s\": %m", vfdP->fileName);
10581085
}
10591086

10601087
/* Unregister it from the resource owner */

0 commit comments

Comments
 (0)
0