Strange path from pgarch_readyXlog()

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

"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