From 9cb7db3f0c1f554cdcbbd97f78a119a19756e6ef Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 28 Apr 2018 17:45:02 -0400 Subject: [PATCH] In AtEOXact_Files, complain if any files remain unclosed at commit. This change makes this module act more like most of our other low-level resource management modules. It's a caller error if something is not explicitly closed by the end of a successful transaction, so issue a WARNING about it. This would not actually have caught the file leak bug fixed in commit 231bcd080, because that was in a transaction-abort path; but it still seems like a good, and pretty cheap, cross-check. Discussion: https://postgr.es/m/152056616579.4966.583293218357089052@wrigleys.postgresql.org --- src/backend/access/transam/xact.c | 6 ++--- src/backend/postmaster/autovacuum.c | 2 +- src/backend/postmaster/bgwriter.c | 2 +- src/backend/postmaster/checkpointer.c | 2 +- src/backend/postmaster/walwriter.c | 2 +- src/backend/storage/file/fd.c | 32 ++++++++++++++++++--------- src/include/storage/fd.h | 2 +- 7 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index c38de0c5fe..f4e5ea84b9 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2123,7 +2123,7 @@ CommitTransaction(void) AtEOXact_on_commit_actions(true); AtEOXact_Namespace(true, is_parallel_worker); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(true); AtEOXact_ComboCid(); AtEOXact_HashTables(true); AtEOXact_PgStat(true); @@ -2401,7 +2401,7 @@ PrepareTransaction(void) AtEOXact_on_commit_actions(true); AtEOXact_Namespace(true, false); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(true); AtEOXact_ComboCid(); AtEOXact_HashTables(true); /* don't call AtEOXact_PgStat here; we fixed pgstat state above */ @@ -2603,7 +2603,7 @@ AbortTransaction(void) AtEOXact_on_commit_actions(false); AtEOXact_Namespace(false, is_parallel_worker); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(false); AtEOXact_ComboCid(); AtEOXact_HashTables(false); AtEOXact_PgStat(false); diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 3b90b16dae..02e6d8131e 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -531,7 +531,7 @@ AutoVacLauncherMain(int argc, char *argv[]) } AtEOXact_Buffers(false); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(false); AtEOXact_HashTables(false); /* diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index b7813d7a4f..d5ce685e54 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -198,7 +198,7 @@ BackgroundWriterMain(void) /* we needn't bother with the other ResourceOwnerRelease phases */ AtEOXact_Buffers(false); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(false); AtEOXact_HashTables(false); /* diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 4b452e7cee..0950ada601 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -282,7 +282,7 @@ CheckpointerMain(void) /* we needn't bother with the other ResourceOwnerRelease phases */ AtEOXact_Buffers(false); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(false); AtEOXact_HashTables(false); /* Warn any waiting backends that the checkpoint failed. */ diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c index 4fa3a3b0aa..688d5b5b80 100644 --- a/src/backend/postmaster/walwriter.c +++ b/src/backend/postmaster/walwriter.c @@ -179,7 +179,7 @@ WalWriterMain(void) /* we needn't bother with the other ResourceOwnerRelease phases */ AtEOXact_Buffers(false); AtEOXact_SMgr(); - AtEOXact_Files(); + AtEOXact_Files(false); AtEOXact_HashTables(false); /* diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index f772dfe93f..afce5dadc0 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -314,7 +314,7 @@ static bool reserveAllocatedDesc(void); static int FreeDesc(AllocateDesc *desc); static void AtProcExit_Files(int code, Datum arg); -static void CleanupTempFiles(bool isProcExit); +static void CleanupTempFiles(bool isCommit, bool isProcExit); static void RemovePgTempFilesInDir(const char *tmpdirname, bool missing_ok, bool unlink_all); static void RemovePgTempRelationFiles(const char *tsdirname); @@ -2902,17 +2902,19 @@ AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid, /* * AtEOXact_Files * - * 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 - * (although they should've been closed already by the ResourceOwner - * cleanup). Furthermore, all "allocated" stdio files are closed. We also - * forget any transaction-local temp tablespace list. + * This routine is called during transaction commit or abort. 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). Furthermore, all "allocated" stdio files are + * closed. We also forget any transaction-local temp tablespace list. + * + * The isCommit flag is used only to decide whether to emit warnings about + * unclosed files. */ void -AtEOXact_Files(void) +AtEOXact_Files(bool isCommit) { - CleanupTempFiles(false); + CleanupTempFiles(isCommit, false); tempTableSpaces = NULL; numTempTableSpaces = -1; } @@ -2926,12 +2928,15 @@ AtEOXact_Files(void) static void AtProcExit_Files(int code, Datum arg) { - CleanupTempFiles(true); + CleanupTempFiles(false, true); } /* * Close temporary files and delete their underlying files. * + * isCommit: if true, this is normal transaction commit, and we don't + * expect any remaining files; warn if there are some. + * * 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 * that's not the case, we are being called for transaction commit/abort @@ -2939,7 +2944,7 @@ AtProcExit_Files(int code, Datum arg) * also clean up "allocated" stdio files, dirs and fds. */ static void -CleanupTempFiles(bool isProcExit) +CleanupTempFiles(bool isCommit, bool isProcExit) { Index i; @@ -2979,6 +2984,11 @@ CleanupTempFiles(bool isProcExit) have_xact_temporary_files = false; } + /* Complain if any allocated files remain open at commit. */ + if (isCommit && numAllocatedDescs > 0) + elog(WARNING, "%d temporary files and directories not closed at end-of-transaction", + numAllocatedDescs); + /* Clean up "allocated" stdio files, dirs and fds. */ while (numAllocatedDescs > 0) FreeDesc(&allocatedDescs[0]); diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index 484339b769..548a832be9 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -123,7 +123,7 @@ extern void SetTempTablespaces(Oid *tableSpaces, int numSpaces); extern bool TempTablespacesAreSet(void); extern int GetTempTablespaces(Oid *tableSpaces, int numSpaces); extern Oid GetNextTempTableSpace(void); -extern void AtEOXact_Files(void); +extern void AtEOXact_Files(bool isCommit); extern void AtEOSubXact_Files(bool isCommit, SubTransactionId mySubid, SubTransactionId parentSubid); extern void RemovePgTempFiles(void);