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
This commit is contained in:
Tom Lane 2021-12-29 17:02:50 -05:00
parent 113fa3945f
commit 1fb17b1903

View File

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