Fix file descriptor leakages in pg_waldump.

TarWALDumpCloseSegment was of the opinion that it didn't need to
do anything.  It was mistaken: it has to close the open file if
any, because nothing else will, leading to a descriptor leak.

In addition, we failed to ensure that any file being read by the
XLogReader machinery gets closed before the atexit callback tries to
cleanup the temporary directory holding spilled WAL files.  While the
file would have been closed already in case of a success exit, this
doesn't happen in case of pg_fatal() exits.  The least messy way
to fix that is to move the atexit function into pg_waldump.c,
where it has easier access to the XLogReaderState pointer and to
WALDumpCloseSegment.

These FD leakages are pretty insignificant on Unix-ish platforms,
but they're a bug on Windows, because they prevent successful cleanup
of the temporary directory for extracted WAL files.  (Windows can't
delete a directory that holds a deleted-but-still-open file.)
This is visible in occasional buildfarm failures.

This code's all new as of commit b15c15139, so no need for back-patch.

Discussion: https://postgr.es/m/374225.1774459521@sss.pgh.pa.us

diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index 96f4d94449f7faf2675efa2c46dbdfa0ae4afab9..b8e9754b7295765b0ac370b7ac8c839f51253018 100644 (file)
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -91,7 +91,6 @@ static ArchivedWALFile *get_archive_wal_entry(const char *fname,
                                                                                          XLogDumpPrivate *privateInfo);
 static bool read_archive_file(XLogDumpPrivate *privateInfo);
 static void setup_tmpwal_dir(const char *waldir);
-static void cleanup_tmpwal_dir_atexit(void);
 
 static FILE *prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo);
 static void perform_tmp_write(const char *fname, StringInfo buf, FILE *file);
@@ -607,23 +606,10 @@ setup_tmpwal_dir(const char *waldir)
        pg_log_debug("created directory \"%s\"", TmpWalSegDir);
 }
 
-/*
- * Remove temporary directory at exit, if any.
- */
-static void
-cleanup_tmpwal_dir_atexit(void)
-{
-       Assert(TmpWalSegDir != NULL);
-
-       rmtree(TmpWalSegDir, true);
-
-       TmpWalSegDir = NULL;
-}
-
 /*
  * Open a file in the temporary spill directory for writing an out-of-order
- * WAL segment, creating the directory and registering the cleanup callback
- * if not already done.  Returns the open file handle.
+ * WAL segment, creating the directory if not already done.
+ * Returns the open file handle.
  */
 static FILE *
 prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo)
@@ -631,15 +617,9 @@ prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo)
        char            fpath[MAXPGPATH];
        FILE       *file;
 
-       /*
-        * Setup temporary directory to store WAL segments and set up an exit
-        * callback to remove it upon completion if not already.
-        */
+       /* Setup temporary directory to store WAL segments, if we didn't already */
        if (unlikely(TmpWalSegDir == NULL))
-       {
                setup_tmpwal_dir(privateInfo->archive_dir);
-               atexit(cleanup_tmpwal_dir_atexit);
-       }
 
        snprintf(fpath, MAXPGPATH, "%s/%s", TmpWalSegDir, fname);
 

diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 630d9859882adcdcbe8eca6240ac2008f07ec23a..6bda3c12aa38316917cfcff28e97494cc09bc5fa 100644 (file)
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -42,6 +42,8 @@ static const char *progname;
 
 static volatile sig_atomic_t time_to_stop = false;
 
+static XLogReaderState *xlogreader_state_cleanup = NULL;
+
 static const RelFileLocator emptyRelFileLocator = {0, 0, 0};
 
 typedef struct XLogDumpConfig
@@ -454,13 +456,14 @@ TarWALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo,
 
 /*
  * pg_waldump's XLogReaderRoutine->segment_close callback to support dumping
- * WAL files from tar archives.  Segment tracking is handled by
- * TarWALDumpReadPage, so no action is needed here.
+ * WAL files from tar archives.  Same as wal_segment_close.
  */
 static void
 TarWALDumpCloseSegment(XLogReaderState *state)
 {
-       /* No action needed */
+       close(state->seg.ws_file);
+       /* need to check errno? */
+       state->seg.ws_file = -1;
 }
 
 /*
@@ -865,6 +868,27 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogStats *stats)
                   total_len, "[100%]");
 }
 
+/*
+ * Remove temporary directory at exit, if any.
+ */
+static void
+cleanup_tmpwal_dir_atexit(void)
+{
+       /*
+        * Before calling rmtree, we must close any open file we have in the temp
+        * directory; else rmdir fails on Windows.
+        */
+       if (xlogreader_state_cleanup != NULL &&
+               xlogreader_state_cleanup->seg.ws_file >= 0)
+               WALDumpCloseSegment(xlogreader_state_cleanup);
+
+       if (TmpWalSegDir != NULL)
+       {
+               rmtree(TmpWalSegDir, true);
+               TmpWalSegDir = NULL;
+       }
+}
+
 static void
 usage(void)
 {
@@ -1383,6 +1407,14 @@ main(int argc, char **argv)
        if (!xlogreader_state)
                pg_fatal("out of memory while allocating a WAL reading processor");
 
+       /*
+        * Set up atexit cleanup of temporary directory.  This must happen before
+        * archive_waldump.c could possibly create the temporary directory.  Also
+        * arm the callback to cleanup the xlogreader state.
+        */
+       atexit(cleanup_tmpwal_dir_atexit);
+       xlogreader_state_cleanup = xlogreader_state;
+
        /* first find a valid recptr to start from */
        first_record = XLogFindNextRecord(xlogreader_state, private.startptr, &errormsg);
 
@@ -1495,6 +1527,12 @@ main(int argc, char **argv)
                                 LSN_FORMAT_ARGS(xlogreader_state->ReadRecPtr),
                                 errormsg);
 
+       /*
+        * Disarm atexit cleanup of open WAL file; XLogReaderFree will close it,
+        * and we don't want the atexit callback trying to touch freed memory.
+        */
+       xlogreader_state_cleanup = NULL;
+
        XLogReaderFree(xlogreader_state);
 
        if (private.archive_name)