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;
 }