Strange path from pgarch_readyXlog()
Hi,
Isn't this a corrupted pathname?
2021-12-29 03:39:55.708 CST [79851:1] WARNING: removal of orphan
archive status file
"pg_wal/archive_status/000000010000000000000003.00000028.backup000000010000000000000004.ready"
failed too many times, will try again later
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=peripatus&dt=2021-12-29%2009%3A20%3A54
On 12/29/21, 12:22 PM, "Thomas Munro" <thomas.munro@gmail.com> wrote:
Isn't this a corrupted pathname?
2021-12-29 03:39:55.708 CST [79851:1] WARNING: removal of orphan
archive status file
"pg_wal/archive_status/000000010000000000000003.00000028.backup000000010000000000000004.ready"
failed too many times, will try again later
I bet this was a simple mistake in beb4e9b.
Nathan
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 434939be9b..b5b0d4e12f 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -113,7 +113,7 @@ static PgArchData *PgArch = NULL;
* is empty, at which point another directory scan must be performed.
*/
static binaryheap *arch_heap = NULL;
-static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS];
+static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS + 1];
static char *arch_files[NUM_FILES_PER_DIRECTORY_SCAN];
static int arch_files_size = 0;
"Bossart, Nathan" <bossartn@amazon.com> writes:
I bet this was a simple mistake in beb4e9b.
-static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS]; +static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS + 1];
Hm, yeah, that looks like a pretty obvious bug.
While we're here, I wonder if we ought to get rid of the static-ness of
these arrays. I realize that they're only eating a few kB, but they're
doing so in every postgres process, when they'll only be used in the
archiver.
regards, tom lane
On 12/29/21, 1:04 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
I bet this was a simple mistake in beb4e9b.
-static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS]; +static char arch_filenames[NUM_FILES_PER_DIRECTORY_SCAN][MAX_XFN_CHARS + 1];Hm, yeah, that looks like a pretty obvious bug.
While we're here, I wonder if we ought to get rid of the static-ness of
these arrays. I realize that they're only eating a few kB, but they're
doing so in every postgres process, when they'll only be used in the
archiver.
This crossed my mind, too. I also think one of the arrays can be
eliminated in favor of just using the heap (after rebuilding with a
reversed comparator). Here is a minimally-tested patch that
demonstrates what I'm thinking.
Nathan
Attachments:
improve-beb4e9b.patchapplication/octet-stream; name=improve-beb4e9b.patchDownload
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 434939be9b..87022b4755 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -106,16 +106,16 @@ static PgArchData *PgArch = NULL;
* archive_status. Minimizing the number of directory scans when there are
* many files to archive can significantly improve archival rate.
*
- * arch_heap is a max-heap that is used during the directory scan to track
- * the highest-priority files to archive. After the directory scan
- * 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.
+ * During the scan of the archive_status directory, arch_heap is a max-heap
+ * (lowest priority file in root node) that stores the highest-priority files to
+ * archive. After the directory scan, arch_heap is rebuilt as a min-heap so
+ * that the highest-priority file is stored in the root node.
+ * pgarch_readyXlog() returns files from arch_heap until it is empty, at which
+ * point another directory scan must be performed.
*/
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;
+static char **arch_filenames = NULL;
+static bool arch_heap_min_heap = false;
/*
* Flags set by interrupt handlers for later service in the main loop.
@@ -231,9 +231,15 @@ PgArchiverMain(void)
*/
PgArch->pgprocno = MyProc->pgprocno;
- /* Initialize our max-heap for prioritizing files to archive. */
+ /* Initialize our heap for prioritizing files to archive. */
arch_heap = binaryheap_allocate(NUM_FILES_PER_DIRECTORY_SCAN,
- ready_file_comparator, NULL);
+ ready_file_comparator,
+ &arch_heap_min_heap);
+
+ /* Initialize our array for storing archive filenames. */
+ arch_filenames = (char **) palloc(NUM_FILES_PER_DIRECTORY_SCAN * sizeof(char *));
+ for (int i = 0; i < NUM_FILES_PER_DIRECTORY_SCAN; ++i)
+ arch_filenames[i] = (char *) palloc(MAX_XFN_CHARS + 1);
pgarch_MainLoop();
@@ -363,7 +369,7 @@ pgarch_ArchiverCopyLoop(void)
char xlog[MAX_XFN_CHARS + 1];
/* force directory scan in the first call to pgarch_readyXlog() */
- arch_files_size = 0;
+ binaryheap_reset(arch_heap);
/*
* loop through all xlogs with archive_status of .ready and archive
@@ -658,7 +664,7 @@ pgarch_readyXlog(char *xlog)
SpinLockRelease(&PgArch->arch_lck);
if (force_dir_scan)
- arch_files_size = 0;
+ binaryheap_reset(arch_heap);
/*
* If we still have stored file names from the previous directory scan,
@@ -666,14 +672,13 @@ 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 (!binaryheap_empty(arch_heap))
{
struct stat st;
char status_file[MAXPGPATH];
char *arch_file;
- arch_files_size--;
- arch_file = arch_files[arch_files_size];
+ arch_file = DatumGetCString(binaryheap_remove_first(arch_heap));
StatusFilePath(status_file, arch_file, ".ready");
if (stat(status_file, &st) == 0)
@@ -687,6 +692,12 @@ pgarch_readyXlog(char *xlog)
errmsg("could not stat file \"%s\": %m", status_file)));
}
+ /*
+ * Assemble a max-heap (lowest priority file in the root node) during the
+ * directory scan.
+ */
+ arch_heap_min_heap = false;
+
/*
* Open the archive status directory and read through the list of files
* with the .ready suffix, looking for the earliest files.
@@ -746,27 +757,18 @@ pgarch_readyXlog(char *xlog)
FreeDir(rldir);
/* If no files were found, simply return. */
- if (arch_heap->bh_size == 0)
+ if (binaryheap_empty(arch_heap))
return false;
/*
- * If we didn't fill the heap, we didn't make it a valid one. Do that
- * now.
+ * Switch the heap to a min-heap so that the highest priority file is stored
+ * in the root node.
*/
- if (arch_heap->bh_size < NUM_FILES_PER_DIRECTORY_SCAN)
- binaryheap_build(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_heap_min_heap = true;
+ binaryheap_build(arch_heap);
/* Return the highest priority file. */
- arch_files_size--;
- strcpy(xlog, arch_files[arch_files_size]);
+ strcpy(xlog, DatumGetCString(binaryheap_remove_first(arch_heap)));
return true;
}
@@ -774,16 +776,25 @@ pgarch_readyXlog(char *xlog)
/*
* ready_file_comparator
*
- * Compares the archival priority of the given files to archive. If "a"
- * has a higher priority than "b", a negative value will be returned. If
- * "b" has a higher priority than "a", a positive value will be returned.
- * If "a" and "b" have equivalent values, 0 will be returned.
+ * Compares the archival priority of the given files to archive. When the extra
+ * argument is true, the comparator returns values for constructing a min-heap
+ * (highest priority file in the root node). In this case, if "a" has a higher
+ * priority than "b", a positive value will be returned. If "b" has a higher
+ * priority than "a", a negative value will be returned. If "a" and "b" have
+ * equivalent values, 0 will be returned.
+ *
+ * When the extra argument is false, the comparator returns values for
+ * constructing a max-heap (lowest priority file in the root node). In this
+ * case, If "a" has a higher priority than "b", a negative value will be
+ * returned. If "b" has a higher priority than "a", a positive value will be
+ * returned. If "a" and "b" have equivalent values, 0 will be returned.
*/
static int
ready_file_comparator(Datum a, Datum b, void *arg)
{
- char *a_str = DatumGetCString(a);
- char *b_str = DatumGetCString(b);
+ bool min_heap = *((bool *) arg);
+ char *a_str = DatumGetCString(min_heap ? b : a);
+ char *b_str = DatumGetCString(min_heap ? a : b);
bool a_history = IsTLHistoryFileName(a_str);
bool b_history = IsTLHistoryFileName(b_str);
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 12/29/21, 1:04 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
While we're here, I wonder if we ought to get rid of the static-ness of
these arrays. I realize that they're only eating a few kB, but they're
doing so in every postgres process, when they'll only be used in the
archiver.
This crossed my mind, too. I also think one of the arrays can be
eliminated in favor of just using the heap (after rebuilding with a
reversed comparator). Here is a minimally-tested patch that
demonstrates what I'm thinking.
I already pushed a patch that de-static-izes those arrays, so this
needs rebased at least. However, now that you mention it it does
seem like maybe the intermediate arch_files[] array could be dropped
in favor of just pulling the next file from the heap.
The need to reverse the heap's sort order seems like a problem
though. I really dislike the kluge you used here with a static flag
that inverts the comparator's sort order behind the back of the
binary-heap mechanism. It seems quite accidental that that doesn't
fall foul of asserts or optimizations in binaryheap.c. For
instance, if binaryheap_build decided it needn't do anything when
bh_has_heap_property is already true, this code would fail. In any
case, we'd need to spend O(n) time inverting the heap's sort order,
so this'd likely be slower than the current code.
On the whole I'm inclined not to bother trying to optimize this
further. The main thing that concerned me is that somebody would
bump up NUM_FILES_PER_DIRECTORY_SCAN to the point where the static
space consumption becomes really problematic, and we've fixed that.
regards, tom lane
On 12/29/21, 3:11 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
This crossed my mind, too. I also think one of the arrays can be
eliminated in favor of just using the heap (after rebuilding with a
reversed comparator). Here is a minimally-tested patch that
demonstrates what I'm thinking.I already pushed a patch that de-static-izes those arrays, so this
needs rebased at least. However, now that you mention it it does
seem like maybe the intermediate arch_files[] array could be dropped
in favor of just pulling the next file from the heap.The need to reverse the heap's sort order seems like a problem
though. I really dislike the kluge you used here with a static flag
that inverts the comparator's sort order behind the back of the
binary-heap mechanism. It seems quite accidental that that doesn't
fall foul of asserts or optimizations in binaryheap.c. For
instance, if binaryheap_build decided it needn't do anything when
bh_has_heap_property is already true, this code would fail. In any
case, we'd need to spend O(n) time inverting the heap's sort order,
so this'd likely be slower than the current code.On the whole I'm inclined not to bother trying to optimize this
further. The main thing that concerned me is that somebody would
bump up NUM_FILES_PER_DIRECTORY_SCAN to the point where the static
space consumption becomes really problematic, and we've fixed that.
Your assessment seems reasonable to me. If there was a better way to
adjust the comparator for the heap, maybe there would be a stronger
case for this approach. I certainly don't think it's worth inventing
something for just this use-case.
Thanks for fixing this!
Nathan