Hi,
On 2021-08-07 09:48:50 -0700, Andres Freund wrote:
> I'm somewhat inclined to split InitFileAccess() into two by separating out
> InitTemporaryFileAccess() or such. InitFileAccess() would continue to happen
> early and register a proc exit hook that errors out when there's a temp file
> (as a backstop for non cassert builds). The new InitTemporaryFileAccess()
> would happen a bit later and schedule CleanupTempFiles() to happen via
> before_shmem_access(). And we add a Assert(!proc_exit_inprogress) to the
> routines for opening a temp file.
Attached is a patch showing how this could look like. Note that the PANIC
should likely not be that but a WARNING, but the PANIC more useful for running
some initial tests...
I'm not sure whether we'd want to continue having the proc exit hook? It seems
to me that asserts would provide a decent enough protection against
introducing new temp files during shutdown.
Alternatively we could make the asserts in OpenTemporaryFile et al
elog(ERROR)s, and be pretty certain that no temp files would be open too late?
Greetings,
Andres Freund
diff --git i/src/include/storage/fd.h w/src/include/storage/fd.h
index 2d843eb9929..34602ae0069 100644
--- i/src/include/storage/fd.h
+++ w/src/include/storage/fd.h
@@ -158,6 +158,7 @@ extern int MakePGDirectory(const char *directoryName);
/* Miscellaneous support routines */
extern void InitFileAccess(void);
+extern void InitTemporaryFileAccess(void);
extern void set_max_safe_fds(void);
extern void closeAllVfds(void);
extern void SetTempTablespaces(Oid *tableSpaces, int numSpaces);
diff --git i/src/backend/storage/file/fd.c w/src/backend/storage/file/fd.c
index df45aa56841..57556a0ac86 100644
--- i/src/backend/storage/file/fd.c
+++ w/src/backend/storage/file/fd.c
@@ -231,6 +231,9 @@ static bool have_xact_temporary_files = false;
*/
static uint64 temporary_files_size = 0;
+/* Temporary file access initialized and not yet shut down? */
+static bool temporary_files_allowed = false;
+
/*
* List of OS handles opened with AllocateFile, AllocateDir and
* OpenTransientFile.
@@ -328,6 +331,7 @@ static bool reserveAllocatedDesc(void);
static int FreeDesc(AllocateDesc *desc);
static void AtProcExit_Files(int code, Datum arg);
+static void BeforeShmemExit_Files(int code, Datum arg);
static void CleanupTempFiles(bool isCommit, bool isProcExit);
static void RemovePgTempRelationFiles(const char *tsdirname);
static void RemovePgTempRelationFilesInDbspace(const char *dbspacedirname);
@@ -868,6 +872,9 @@ durable_rename_excl(const char *oldfile, const char *newfile, int elevel)
*
* This is called during either normal or standalone backend start.
* It is *not* called in the postmaster.
+ *
+ * Note that this does not initialize temporary file access, that is
+ * separately initialized via InitTemporaryFileAccess().
*/
void
InitFileAccess(void)
@@ -886,9 +893,34 @@ InitFileAccess(void)
SizeVfdCache = 1;
- /* register proc-exit hook to ensure temp files are dropped at exit */
+ /*
+ * Register proc-exit hook to ensure temp files are dropped at exit. This
+ * serves as a backstop to BeforeShmemExit_Files() in case somebody
+ * creates a temp file in a shutdown hook.
+ */
on_proc_exit(AtProcExit_Files, 0);
- before_shmem_exit(AtProcExit_Files, 0);
+}
+
+/*
+ * InitTemporaryFileAccess --- initialize temporary file access during startup
+ *
+ * This is called during either normal or standalone backend start.
+ * It is *not* called in the postmaster.
+ */
+void
+InitTemporaryFileAccess(void)
+{
+ Assert(SizeVfdCache != 0); /* InitFileAccess() needs to have run*/
+
+ /*
+ * Register before-shmem-exit hook to ensure temp files are dropped while
+ * we can still report stats.
+ */
+ before_shmem_exit(BeforeShmemExit_Files, 0);
+
+#ifdef USE_ASSERT_CHECKING
+ temporary_files_allowed = true;
+#endif
}
/*
@@ -1671,6 +1703,8 @@ OpenTemporaryFile(bool interXact)
{
File file = 0;
+ Assert(temporary_files_allowed); /* check temp file access is up */
+
/*
* Make sure the current resource owner has space for this File before we
* open it, if we'll be registering it below.
@@ -1806,6 +1840,8 @@ PathNameCreateTemporaryFile(const char *path, bool error_on_failure)
{
File file;
+ Assert(temporary_files_allowed); /* check temp file access is up */
+
ResourceOwnerEnlargeFiles(CurrentResourceOwner);
/*
@@ -1844,6 +1880,8 @@ PathNameOpenTemporaryFile(const char *path, int mode)
{
File file;
+ Assert(temporary_files_allowed); /* check temp file access is up */
+
ResourceOwnerEnlargeFiles(CurrentResourceOwner);
file = PathNameOpenFile(path, mode | PG_BINARY);
@@ -3004,11 +3042,28 @@ AtEOXact_Files(bool isCommit)
numTempTableSpaces = -1;
}
+/*
+ * BeforeShmemExit_Files
+ *
+ * before_shmem_access hook to clean up temp files during backend shutdown.
+ * Here, we want to clean up *all* temp files including interXact ones.
+ */
+static void
+BeforeShmemExit_Files(int code, Datum arg)
+{
+ CleanupTempFiles(false, true);
+
+ /* prevent further temp files from being created */
+ temporary_files_allowed = false;
+}
+
/*
* AtProcExit_Files
*
* on_proc_exit hook to clean up temp files during backend shutdown.
- * Here, we want to clean up *all* temp files including interXact ones.
+ *
+ * They all should have been cleaned up during BeforeShmemExit_Files and is
+ * just a backstop / debugging aid.
*/
static void
AtProcExit_Files(int code, Datum arg)
@@ -3027,6 +3082,10 @@ AtProcExit_Files(int code, Datum arg)
* that's not the case, we are being called for transaction commit/abort
* and should only remove transaction-local temp files. In either case,
* also clean up "allocated" stdio files, dirs and fds.
+ *
+ * This will be called twice during shutdown. Once from
+ * BeforeShmemExit_Files(), once from AtProcExit_Files(). The latter should
+ * not find any temporary files and is just a backstop / debugging aid.
*/
static void
CleanupTempFiles(bool isCommit, bool isProcExit)
@@ -3055,7 +3114,14 @@ CleanupTempFiles(bool isCommit, bool isProcExit)
* debugging cross-check.
*/
if (isProcExit)
+ {
+ /* FIXME: replace PANIC with WARNING before commit */
+ if (!temporary_files_allowed)
+ elog(PANIC,
+ "temporary file %s open while temporary file access is not allowed",
+ VfdCache[i].fileName);
FileClose(i);
+ }
else if (fdstate & FD_CLOSE_AT_EOXACT)
{
elog(WARNING,
diff --git i/src/backend/utils/init/postinit.c w/src/backend/utils/init/postinit.c
index 87dc060b201..5089dd43ae2 100644
--- i/src/backend/utils/init/postinit.c
+++ w/src/backend/utils/init/postinit.c
@@ -517,6 +517,12 @@ BaseInit(void)
*/
DebugFileOpen();
+ /*
+ * Initialize file access. Done early so other subsystems can access
+ * files.
+ */
+ InitFileAccess();
+
/*
* Initialize statistics reporting. This needs to happen early to ensure
* that pgstat's shutdown callback runs after the shutdown callbacks of
@@ -525,11 +531,16 @@ BaseInit(void)
*/
pgstat_initialize();
- /* Do local initialization of file, storage and buffer managers */
- InitFileAccess();
+ /* Do local initialization of storage and buffer managers */
InitSync();
smgrinit();
InitBufferPoolAccess();
+
+ /*
+ * Initialize temporary file access after pgstat, so that the temorary
+ * file shutdown hook can report temporary file statistics.
+ */
+ InitTemporaryFileAccess();
}
