Skip to content

Commit 1fb17b1

Browse files
tglsfdctglsfdc
committed
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 beb4e9b, so no back-patch needed. Discussion: https://postgr.es/m/CA+hUKGLHAjHuKuwtzsW7uMJF4BVPcQRL-UMZG_HM-g0y7yLkUg@mail.gmail.com
1 parent 113fa39 commit 1fb17b1

File tree

1 file changed

+43
-27
lines changed

1 file changed

+43
-27
lines changed

‎src/backend/postmaster/pgarch.c

+43-27
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,20 @@ static PgArchData *PgArch = NULL;
111111
* completes, the file names are stored in ascending order of priority in
112112
* arch_files. pgarch_readyXlog() returns files from arch_files until it
113113
* is empty, at which point another directory scan must be performed.
114+
*
115+
* We only need this data in the archiver process, so make it a palloc'd
116+
* struct rather than a bunch of static arrays.
114117
*/
115-
static binaryheap *arch_heap = NULL;
116-
static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS];
117-
static char *arch_files[NUM_FILES_PER_DIRECTORY_SCAN];
118-
static int arch_files_size = 0;
118+
struct arch_files_state
119+
{
120+
binaryheap *arch_heap;
121+
int arch_files_size; /* number of live entries in arch_files[] */
122+
char *arch_files[NUM_FILES_PER_DIRECTORY_SCAN];
123+
/* buffers underlying heap, and later arch_files[], entries: */
124+
char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS + 1];
125+
};
126+
127+
static struct arch_files_state *arch_files = NULL;
119128

120129
/*
121130
* Flags set by interrupt handlers for later service in the main loop.
@@ -231,9 +240,13 @@ PgArchiverMain(void)
231240
*/
232241
PgArch->pgprocno = MyProc->pgprocno;
233242

243+
/* Create workspace for pgarch_readyXlog() */
244+
arch_files = palloc(sizeof(struct arch_files_state));
245+
arch_files->arch_files_size = 0;
246+
234247
/* Initialize our max-heap for prioritizing files to archive. */
235-
arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
236-
ready_file_comparator, NULL);
248+
arch_files->arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
249+
ready_file_comparator, NULL);
237250

238251
pgarch_MainLoop();
239252

@@ -363,7 +376,7 @@ pgarch_ArchiverCopyLoop(void)
363376
char xlog[MAX_XFN_CHARS + 1];
364377

365378
/* force directory scan in the first call to pgarch_readyXlog() */
366-
arch_files_size = 0;
379+
arch_files->arch_files_size = 0;
367380

368381
/*
369382
* loop through all xlogs with archive_status of .ready and archive
@@ -658,22 +671,22 @@ pgarch_readyXlog(char *xlog)
658671
SpinLockRelease(&PgArch->arch_lck);
659672

660673
if (force_dir_scan)
661-
arch_files_size = 0;
674+
arch_files->arch_files_size = 0;
662675

663676
/*
664677
* If we still have stored file names from the previous directory scan,
665678
* try to return one of those. We check to make sure the status file
666679
* is still present, as the archive_command for a previous file may
667680
* have already marked it done.
668681
*/
669-
while (arch_files_size > 0)
682+
while (arch_files->arch_files_size > 0)
670683
{
671684
struct stat st;
672685
char status_file[MAXPGPATH];
673686
char *arch_file;
674687

675-
arch_files_size--;
676-
arch_file = arch_files[arch_files_size];
688+
arch_files->arch_files_size--;
689+
arch_file = arch_files->arch_files[arch_files->arch_files_size];
677690
StatusFilePath(status_file, arch_file, ".ready");
678691

679692
if (stat(status_file, &st) == 0)
@@ -687,6 +700,9 @@ pgarch_readyXlog(char *xlog)
687700
errmsg("could not stat file \"%s\": %m", status_file)));
688701
}
689702

703+
/* arch_heap is probably empty, but let's make sure */
704+
binaryheap_reset(arch_files->arch_heap);
705+
690706
/*
691707
* Open the archive status directory and read through the list of files
692708
* with the .ready suffix, looking for the earliest files.
@@ -720,53 +736,53 @@ pgarch_readyXlog(char *xlog)
720736
/*
721737
* Store the file in our max-heap if it has a high enough priority.
722738
*/
723-
if (arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
739+
if (arch_files->arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
724740
{
725741
/* If the heap isn't full yet, quickly add it. */
726-
arch_file = arch_filenames[arch_heap->bh_size];
742+
arch_file = arch_files->arch_filenames[arch_files->arch_heap->bh_size];
727743
strcpy(arch_file, basename);
728-
binaryheap_add_unordered(arch_heap, CStringGetDatum(arch_file));
744+
binaryheap_add_unordered(arch_files->arch_heap, CStringGetDatum(arch_file));
729745

730746
/* If we just filled the heap, make it a valid one. */
731-
if (arch_heap->bh_size == NUM_FILES_PER_DIRECTORY_SCAN)
732-
binaryheap_build(arch_heap);
747+
if (arch_files->arch_heap->bh_size == NUM_FILES_PER_DIRECTORY_SCAN)
748+
binaryheap_build(arch_files->arch_heap);
733749
}
734-
else if (ready_file_comparator(binaryheap_first(arch_heap),
750+
else if (ready_file_comparator(binaryheap_first(arch_files->arch_heap),
735751
CStringGetDatum(basename), NULL) > 0)
736752
{
737753
/*
738754
* Remove the lowest priority file and add the current one to
739755
* the heap.
740756
*/
741-
arch_file = DatumGetCString(binaryheap_remove_first(arch_heap));
757+
arch_file = DatumGetCString(binaryheap_remove_first(arch_files->arch_heap));
742758
strcpy(arch_file, basename);
743-
binaryheap_add(arch_heap, CStringGetDatum(arch_file));
759+
binaryheap_add(arch_files->arch_heap, CStringGetDatum(arch_file));
744760
}
745761
}
746762
FreeDir(rldir);
747763

748764
/* If no files were found, simply return. */
749-
if (arch_heap->bh_size == 0)
765+
if (arch_files->arch_heap->bh_size == 0)
750766
return false;
751767

752768
/*
753769
* If we didn't fill the heap, we didn't make it a valid one. Do that
754770
* now.
755771
*/
756-
if (arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
757-
binaryheap_build(arch_heap);
772+
if (arch_files->arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
773+
binaryheap_build(arch_files->arch_heap);
758774

759775
/*
760776
* Fill arch_files array with the files to archive in ascending order
761777
* of priority.
762778
*/
763-
arch_files_size = arch_heap->bh_size;
764-
for (int i = 0; i < arch_files_size; i++)
765-
arch_files[i] = DatumGetCString(binaryheap_remove_first(arch_heap));
779+
arch_files->arch_files_size = arch_files->arch_heap->bh_size;
780+
for (int i = 0; i < arch_files->arch_files_size; i++)
781+
arch_files->arch_files[i] = DatumGetCString(binaryheap_remove_first(arch_files->arch_heap));
766782

767783
/* Return the highest priority file. */
768-
arch_files_size--;
769-
strcpy(xlog, arch_files[arch_files_size]);
784+
arch_files->arch_files_size--;
785+
strcpy(xlog, arch_files->arch_files[arch_files->arch_files_size]);
770786

771787
return true;
772788
}

0 commit comments

Comments
 (0)