Fix issues in pgarch's new directory-scanning logic.

The arch_filenames[] array elements were one byte too small, so that
a maximum-length filename would get corrupted if another entry
were made after it.  (Noted by Thomas Munro, fix by Nathan Bossart.)

Move these arrays into a palloc'd struct, so that we aren't wasting
a few kilobytes of static data in each non-archiver process.

Add a binaryheap_reset() call to make it plain that we start the
directory scan with an empty heap.  I don't think there's any live
bug of that sort, but it seems fragile, and this is very cheap
insurance.

Cleanup for commit beb4e9ba1, so no back-patch needed.

Discussion: https://postgr.es/m/CA+hUKGLHAjHuKuwtzsW7uMJF4BVPcQRL-UMZG_HM-g0y7yLkUg@mail.gmail.com

diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 434939be9bc2100b16c859496b6d7b356b3e6e6e..5b6bf9f4e0735c114b300df4ef722366c9e58268 100644 (file)
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -111,11 +111,20 @@ static PgArchData *PgArch = NULL;
  * completes, the file names are stored in ascending order of priority in
  * arch_files.  pgarch_readyXlog() returns files from arch_files until it
  * is empty, at which point another directory scan must be performed.
+ *
+ * We only need this data in the archiver process, so make it a palloc'd
+ * struct rather than a bunch of static arrays.
  */
-static binaryheap *arch_heap = NULL;
-static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS];
-static char *arch_files[NUM_FILES_PER_DIRECTORY_SCAN];
-static int arch_files_size = 0;
+struct arch_files_state
+{
+       binaryheap *arch_heap;
+       int                     arch_files_size;        /* number of live entries in arch_files[] */
+       char       *arch_files[NUM_FILES_PER_DIRECTORY_SCAN];
+       /* buffers underlying heap, and later arch_files[], entries: */
+       char            arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS + 1];
+};
+
+static struct arch_files_state *arch_files = NULL;
 
 /*
  * Flags set by interrupt handlers for later service in the main loop.
@@ -231,9 +240,13 @@ PgArchiverMain(void)
         */
        PgArch->pgprocno = MyProc->pgprocno;
 
+       /* Create workspace for pgarch_readyXlog() */
+       arch_files = palloc(sizeof(struct arch_files_state));
+       arch_files->arch_files_size = 0;
+
        /* Initialize our max-heap for prioritizing files to archive. */
-       arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
-                                                                       ready_file_comparator, NULL);
+       arch_files->arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
+                                                                                               ready_file_comparator, NULL);
 
        pgarch_MainLoop();
 
@@ -363,7 +376,7 @@ pgarch_ArchiverCopyLoop(void)
        char            xlog[MAX_XFN_CHARS + 1];
 
        /* force directory scan in the first call to pgarch_readyXlog() */
-       arch_files_size = 0;
+       arch_files->arch_files_size = 0;
 
        /*
         * loop through all xlogs with archive_status of .ready and archive
@@ -658,7 +671,7 @@ pgarch_readyXlog(char *xlog)
        SpinLockRelease(&PgArch->arch_lck);
 
        if (force_dir_scan)
-               arch_files_size = 0;
+               arch_files->arch_files_size = 0;
 
        /*
         * If we still have stored file names from the previous directory scan,
@@ -666,14 +679,14 @@ pgarch_readyXlog(char *xlog)
         * is still present, as the archive_command for a previous file may
         * have already marked it done.
         */
-       while (arch_files_size > 0)
+       while (arch_files->arch_files_size > 0)
        {
                struct stat     st;
                char            status_file[MAXPGPATH];
                char       *arch_file;
 
-               arch_files_size--;
-               arch_file = arch_files[arch_files_size];
+               arch_files->arch_files_size--;
+               arch_file = arch_files->arch_files[arch_files->arch_files_size];
                StatusFilePath(status_file, arch_file, ".ready");
 
                if (stat(status_file, &st) == 0)
@@ -687,6 +700,9 @@ pgarch_readyXlog(char *xlog)
                                         errmsg("could not stat file \"%s\": %m", status_file)));
        }
 
+       /* arch_heap is probably empty, but let's make sure */
+       binaryheap_reset(arch_files->arch_heap);
+
        /*
         * Open the archive status directory and read through the list of files
         * with the .ready suffix, looking for the earliest files.
@@ -720,53 +736,53 @@ pgarch_readyXlog(char *xlog)
                /*
                 * Store the file in our max-heap if it has a high enough priority.
                 */
-               if (arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
+               if (arch_files->arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
                {
                        /* If the heap isn't full yet, quickly add it. */
-                       arch_file = arch_filenames[arch_heap->bh_size];
+                       arch_file = arch_files->arch_filenames[arch_files->arch_heap->bh_size];
                        strcpy(arch_file, basename);
-                       binaryheap_add_unordered(arch_heap, CStringGetDatum(arch_file));
+                       binaryheap_add_unordered(arch_files->arch_heap, CStringGetDatum(arch_file));
 
                        /* If we just filled the heap, make it a valid one. */
-                       if (arch_heap->bh_size == NUM_FILES_PER_DIRECTORY_SCAN)
-                               binaryheap_build(arch_heap);
+                       if (arch_files->arch_heap->bh_size == NUM_FILES_PER_DIRECTORY_SCAN)
+                               binaryheap_build(arch_files->arch_heap);
                }
-               else if (ready_file_comparator(binaryheap_first(arch_heap),
+               else if (ready_file_comparator(binaryheap_first(arch_files->arch_heap),
                                                                           CStringGetDatum(basename), NULL) > 0)
                {
                        /*
                         * Remove the lowest priority file and add the current one to
                         * the heap.
                         */
-                       arch_file = DatumGetCString(binaryheap_remove_first(arch_heap));
+                       arch_file = DatumGetCString(binaryheap_remove_first(arch_files->arch_heap));
                        strcpy(arch_file, basename);
-                       binaryheap_add(arch_heap, CStringGetDatum(arch_file));
+                       binaryheap_add(arch_files->arch_heap, CStringGetDatum(arch_file));
                }
        }
        FreeDir(rldir);
 
        /* If no files were found, simply return. */
-       if (arch_heap->bh_size == 0)
+       if (arch_files->arch_heap->bh_size == 0)
                return false;
 
        /*
         * If we didn't fill the heap, we didn't make it a valid one.  Do that
         * now.
         */
-       if (arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
-               binaryheap_build(arch_heap);
+       if (arch_files->arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
+               binaryheap_build(arch_files->arch_heap);
 
        /*
         * Fill arch_files array with the files to archive in ascending order
         * of priority.
         */
-       arch_files_size = arch_heap->bh_size;
-       for (int i = 0; i < arch_files_size; i++)
-               arch_files[i] = DatumGetCString(binaryheap_remove_first(arch_heap));
+       arch_files->arch_files_size = arch_files->arch_heap->bh_size;
+       for (int i = 0; i < arch_files->arch_files_size; i++)
+               arch_files->arch_files[i] = DatumGetCString(binaryheap_remove_first(arch_files->arch_heap));
 
        /* Return the highest priority file. */
-       arch_files_size--;
-       strcpy(xlog, arch_files[arch_files_size]);
+       arch_files->arch_files_size--;
+       strcpy(xlog, arch_files->arch_files[arch_files->arch_files_size]);
 
        return true;
 }