Subject: pg_waldump: support decoding of WAL inside tarfile

From: Amul Sul <[email-masked]>
Date: Thu, 7 Aug 2025 19:47:08 +0530

Hi All,

Attaching patch to support a new feature that let pg_waldump decode
WAL files directly from a tar archive. This was worked to address a
limitation in pg_verifybackup[1], which couldn't parse WAL files from
tar-formatted backups.

The implementation will align with pg_waldump's existing xlogreader
design, which uses three callback functions to manage WAL segments:
open, read, and close. For tar archives, however, the approach will be
simpler. Instead of using separate callbacks for opening and closing,
the tar archive will be opened once at the start and closed explicitly
at the end.

The core logic will be in the WAL page reading callback. When
xlogreader requests a new WAL page, this callback will be invoked. It
will then call the archive streamer routine to read the WAL data from
the tar archive into a buffer. This data will then be copied into
xlogreader's own buffer, completing the read.

Essentially, this is plumbing work: the new code will be responsible
for getting WAL data from the tar archive and feeding it to the
existing xlogreader. All other WAL page and record decoding logic,
which is already robust within xlogreader, will be reused as is.

This feature is being implemented in a series of patches as:

- Refactoring: The first few patches (0001-0004) are dedicated to
refactoring and minor code changes.

- 005: This patch introduces the core functionality for pg_waldump to
read WAL from a tar archive using the same archive streamer
(fe_utils/astreamer.h) used in pg_verifybackup. This version requires
WAL files in the archive to be in sequential order.

- 006: This patch removes the sequential order restriction. If
pg_waldump encounters an out-of-order WAL file, it writes the file to
a temporary directory. The utility will then continue decoding and
read from this temporary location later.

- 007 and onwards: These patches will update pg_verifybackup to remove the
restriction on WAL parsing for tar-formatted backups. 008 patch renames the
"--wal-directory" switch to "--wal-path" to make it more generic, allowing
it accepts a directory path or a tar archive path.

-----------------------------------
Known Issues & Status:
-----------------------------------
- Timeline Switching: The current implementation in patch 006 does not
correctly handle timeline switching. This is a known issue, especially
when a timeline change occurs on a WAL file that has been written to a
temporary location.

- Testing: Local regression tests on CentOS and macOS M4 are passing.
However, some tests on macOS Sonoma (specifically 008_untar.pl and
010_client_untar.pl) are failing in the GitHub workflow with a "WAL
parsing failed for timeline 1" error. This issue is currently being
investigated.

Please take a look at the attached patch and let me know your
thoughts. This is an initial version, and I am making incremental
improvements to address known issues and limitations.

1] https://git.postgresql.org/pg/commitdiff/8dfd3129027969fdd2d9d294220c867d2efd84aa

--
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

---

From: Amul Sul <[email-masked]>
Date: Mon, 25 Aug 2025 17:58:44 +0530

On Thu, Aug 7, 2025 at 7:47 PM Amul Sul <[email-masked]> wrote:
> [....]

This is still pending and will be addressed in the next version.
Therefore, patch 0006 remains marked as WIP.

> parsing failed for timeline 1" error. This issue is currently being

This has been fixed in the attached version; all GitHub workflow tests
are now fine.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Tue, 26 Aug 2025 17:22:20 +0530

> Therefore, patch 0006 remains marked as WIP.

After testing pg_waldump, I have realised that my previous
understanding of its timeline handling was incorrect. I had mistakenly
assumed by reading xlogreader code that it would use the same
timeline-switching logic found in xlogreader, without first verifying
this behavior. In testing, I found that pg_waldump does not follow
timeline switches. Instead, it expects all WAL files to be from a
single timeline, which is either specified by the user or determined
from the starting segment or default 1.

This is a positive finding, as it means we don't need to make
significant changes to align pg_waldump's current behavior. The
attached patches are now complete and no longer works in progress --
read for review. Additionally, I've dropped patch v2-0004 because it is
no longer necessary. The primary patches that implement the proposed
feature are now 0004 and 0005 in the attached set.

Regards,
Amul

---

From: Jakub Wartak <[email-masked]>
Date: Mon, 8 Sep 2025 15:37:02 +0200

On Tue, Aug 26, 2025 at 1:53 PM Amul Sul <[email-masked]> wrote:
[..patch]

Hi Amul!

0001: LGTM, maybe I would just slightly enhance the commit message
("This is in preparation for adding a second source file to this
directory.") -- maye bit a bit more verbose or use a message from
0002?
0002: LGTM
0003: LGTM

Tested here (after partial patch apply, and test suite did work fine).

0004:

    a.  Why should it be necessary to provide startLSN (-s) ? Couldn't
it autodetect the first WAL (tar file) inside and just use that with
some info message?
    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar
    pg_waldump: error: no start WAL location given

    b. Why would it like to open "blah" dir if I wanted that "blah"
segment from the archive? Shouldn't it tell that it was looking in the
archive and couldn find it inside?
    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar blah
    pg_waldump: error: could not open file "blah": Not a directory

    c. It doesnt work when using SEGSTART, but it's there:
    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar
000000010000000000000059
    pg_waldump: error: could not open file "000000010000000000000059":
Not a directory
    $ tar tf /tmp/base/pg_wal.tar | head -1
    000000010000000000000059

    d.     I've later noticed that follow-up patches seem to use the
-s switch and there it seems to work OK. The above SEGSTART issue was
not detected, probably because tests need to be extended cover  of
segment name rather than just --start LSN (see test_pg_waldump):
    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar --stats
-s 0/59000358
    pg_waldump: first record is after 0/59000358, at 0/590003E8,
skipping over 144 bytes
    WAL statistics between 0/590003E8 and 0/61000000:
    [..]

    e. Code around`if (walpath == NULL && directory != NULL)` needs
some comments.

    f. Code around `if (fname != NULL && is_tar_file(fname,
&compression))` , so if fname is WAL segment here
(00000001000000000000005A) and we do check again if that has been
tar-ed (is_tar_file())? Why?

    g. Just a question: the commit message says `Note that this patch
requires that the WAL files within the archive be in sequential order;
an error will be reported otherwise`. I'm wondering if such
occurrences are known to be happening in the wild? Or is it just an
assumption that if someone would modify the tar somehow? (either way
we could just add a reason why we need to handle such a case if we
know -- is manual alternation the only source of such state?). For the
record, I've tested crafting custom archives with out of sequence WAL
archives and the code seems to work (it was done using: tar --append
-f pg_wal.tar --format=ustar ..)

    h. Anyway, in case of typo/wrong LSN, 0004 emits wrong error
message I think:

    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar --stats
-s 0/50000358
    pg_waldump: error: WAL files are not archived in sequential order
    pg_waldump: detail: Expecting segment number 80 but found 89.

    it's just that the 50000358 LSN above is below the minimal LSN
present in the WAL segments (first segment is 000000010000000000000059
there, i've just intentionally provided a bad value 50.. as a typo and
it causes the wrong message). Now it might not be an issue as with
0005 patch the same test behaves OK (`pg_waldump: error: could not
find a valid record after 0/50000358`). It is just relevant if this
would be committed not all at once.

    i. If I give wrong --timeline=999 to pg_waldump it fails with
misleading error message: could not read WAL data from "pg_wal.tar"
archive: read -1 of 8192

0005:
    a. I'm wondering if we shouldn't log (to stderr?) some kind of
notification message (just once) that non-sequential WAL files were
discovered and that pg_waldump is starting to write to $somewhere as
it may be causing bigger I/O than anticipated when running the
command. This can easily help when troubleshooting why it is not fast,
and also having set TMPDIR to usually /tmp can be slow or too small.

    b. IMHO member_prepare_tmp_write() / get_tmp_wal_file_path() with
TMPDIR can be prone to symlink attack. Consider setting TMPDIR=/tmp .
We are writing to e.g. /tmp/<WALsegment>.waldump.tmp in 0004 , but
that path is completely guessable. If an attacker prepares some
symlinks and links those to some other places, I think the code will
happily open and overwrite the contents of the rogue symlink. I think
using mkstemp(3)/tmpfile(3) would be a safer choice if TMPDIR needs to
be in play. Consider that pg_waldump can be run as root (there's no
mechanism preventing it from being used that way).

    c. IMHO that unlink() might be not guaranteed to always remove
files, as in case of any trouble and exit() , those files might be
left over. I think we need some atexit() handlers. This can be
triggered with combo of options of nonsequential files in tar + wrong
LSN given:

    $ tar tf pg_wal.tar
    00000001000000000000005A
    00000001000000000000005B
    00000001000000000000005C
    [..]
    000000010000000000000060
    000000010000000000000059 <-- out of order, appended last
    $ ls -lh 0*
    ls: cannot access '0*': No such file or directory
    $ /usr/pgsql19/bin/pg_waldump --path=/tmp/ble/pg_wal.tar --stats
-s 0/10000358 #wrong LSN
    pg_waldump: error: could not find a valid record after 0/10000358
    $ ls -lh 0*
    -rw------- 1 postgres postgres 16M Sep  8 14:44
000000010000000000000059.waldump.tmp
    -rw------- 1 postgres postgres 16M Sep  8 14:44
00000001000000000000005A.waldump.tmp
    [..]

0006: LGTM

0007:
    a. Commit message says `Future patches to pg_waldump will enable
it to decode WAL directly` , but those pg_waldump are earlier patches,
right?

    b. pg_verifybackup should print some info with --progress that it
is spawning pg_waldump (pg_verifybackup --progress mode does not
display anything related to verifing WALs, but it could)

    c. I'm wondering, but pg_waldump seems to be not complaining if
--end=LSN is made into such a future that it doesn't exist. E.g. If
the latest WAL segment is 60 (with end LSN 0/60A77A59), but I run
pg_waldump `--end=0/7000000` , it will return code 0 and nothing on
stderr. So how sure are we that the necessary WAL segments (as per
backup_manifest) are actually inside the tar? It's supposed to be
verified, but it isn't for this use case? Same happens if craft
special tar and remove just one WAL segment from pg_wal.tar (simulate
missing WAL segment), but ask the pg_verifybackup/pg_waldump to verify
it to exact last LSN sequence, e.g.:

    $ /usr/pgsql19/bin/pg_waldump --quiet
--path=/tmp/missing/pg_wal.tar --timeline=1 --start=0/59000028
--end=0/60A77A58 && echo OK # but it is not OK
    OK
    $ /usr/pgsql19/bin/pg_waldump --stats
--path=/tmp/missing/pg_wal.tar --timeline=1 --start=0/59000028
--end=0/60A77A58
    WAL statistics between 0/59000028 and 0/5CFFFFD0: # <-- 0/5C LSN
maximum detected
    [..]

    Notice it has read till 0/5C (but I've asked till 0/60), because
I've removed 0D:
    $ tar tf /tmp/missing/pg_wal.tar| grep ^0
    000000010000000000000059
    00000001000000000000005A
    00000001000000000000005B
    00000001000000000000005C
    00000001000000000000005E <-- missing 5D

    Yet it reported no errors.

0008:
    LGTM

Another open question I have is this: shouldn't backup_manifest come
with CRC checksum for the archived WALs? Or does that guarantee that
backup_manifest WAL-Ranges are present in pg_wal.tar is good enough
because individual WAL files are CRC-protected itself?

-J.

---

From: Amul Sul <[email-masked]>
Date: Fri, 12 Sep 2025 16:25:28 +0530

> On Tue, Aug 26, 2025 at 1:53 PM Amul Sul <[email-masked]> wrote:

Thanks for your review. I'm replying to a few of your comments now,
but for the rest, I need to think about them. I'm kind of in agreement
with some of them for the fix, but I won't be able to spend time on
that next week due to official travel. I'll try to get back as soon as
possible after that.

>     pg_waldump: error: no start WAL location given

There are two reasons. First,  existing pg_waldump
--path=some_directory would result in the same error. Second, it would
force us to re-read the archive twice just to locate the first WAL
segment, which is inefficient.

>     $ /usr/pgsql19/bin/pg_waldump --path=/tmp/base/pg_wal.tar

I don't believe this is the correct use case. The WAL files are inside
a tar archive, and the requirement is to use a starting LSN and a
timeline (if not the default).

>     WAL statistics between 0/590003E8 and 0/61000000:

Hope previous reasoning makes sense to you.

>     e. Code around`if (walpath == NULL && directory != NULL)` needs

I think this is an existing one.

> tar-ed (is_tar_file())? Why?

Again, how?

> -f pg_wal.tar --format=ustar ..)

This is an almost nonexistent occurrence. While pg_basebackup archives
WAL files in sequential order, we don't have an explicit code to
enforce that order within it. Furthermore, since we can't control how
external tools might handle the files, this extra precaution is
necessary.

> because individual WAL files are CRC-protected itself?

I don't know, I have to check pg_verifybackup.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Fri, 12 Sep 2025 14:28:15 -0400

Here are some review comments on v3-0004:

In general, I think this looks pretty nice, but I think it needs more
cleanup and polishing.

There doesn't seem to be any reason for
astreamer_waldump_content_new() to take an astreamer *next argument.
If you look at astreamer.h, you'll see that some astreamer_BLAH_new()
functions take such an argument, and others don't. The ones that do
forward their input to another astreamer; the ones that don't, like
astreamer_plain_writer_new(), send it somewhere else. AFAICT, this
astreamer is never going to send its output to another astreamer, so
there's no reason for this argument.

I'm also a little confused by the choice of the name
astreamer_waldump_content_new(). I would have thought this would be
something like astreamer_waldump_new() or astreamer_xlogreader_new().
The word "content" doesn't seem to me to be adding much here, and it
invites confusion with the "content" callback.

I think you can merge setup_astreamer() into
init_tar_archive_reader(). The only other caller is
verify_tar_archive(), but that does exactly the same additional steps
as init_tar_archive_reader(), as far as I can see.

The return statement for astreamer_wal_read is really odd:

+       return (count - nbytes) ? (count - nbytes) : -1;

Since 0 is false in C, this is equivalent to: count != nbytes ? count
- nbytes : -1, but it's a strange way to write it. What makes it even
stranger is that it seems as though the intention here is to count the
number of bytes read, but you do that by taking the number of bytes
requested (count) and subtracting the number of bytes we didn't manage
to read (nbytes); and then you just up and return -1 instead of 0
whenever the answer would have been zero. This is all lacking in
comments and seems a bit more confusing than it needs to be. So my
suggestions are:

1. Consider redefining nbytes to be the number of bytes that you have
read instead of the number of bytes you haven't read. So the loop in
this function would be while (nbytes < count) instead of while (nbytes
> 0).

2. If you need to map 0 to -1, consider having the caller do this
instead of putting that inside this function.

3. Add a comment saying what the return value is supposed to be".

If you do both 1 and 2, then the return statement can just say "return
nbytes;" and the comment can say "Returns the number of bytes
successfully read."

I would suggest changing the name of the variable from "readBuff" to
"readBuf". There are no existing uses of readBuff in the code base.

I think this comment also needs improvement:

+               /*
+                * Ignore existing data if the required target page
has not yet been
+                * read.
+                */
+               if (recptr >= endPtr)
+               {
+                       len = 0;
+
+                       /* Reset the buffer */
+                       resetStringInfo(astreamer_buf);
+               }

This comment is problematic for a few reasons. First, we're not
ignoring the existing data: we're throwing it out. Second, the comment
doesn't say why we're doing what we're doing, only that we're doing
it. Here's my guess at the actual explanation -- please correct me if
I'm wrong: "pg_waldump never reads the same WAL bytes more than once,
so if we're now being asked for data beyond the end of what we've
already read, that means none of the data we currently have in the
buffer will ever be consulted again. So, we can discard the existing
buffer contents and start over." By the way, if this explanation is
correct, it might be nice to add an assertion someplace that verifies
it, like asserting that we're always reading from an LSN greater than
or equal to (or exactly equal to?) the LSN immediately following the
last data we read.

In general, I wonder whether there's a way to make the separation of
concerns between astreamer_wal_read() and TarWALDumpReadPage()
cleaner. Right now, the latter is basically a stub, but I'm not sure
that is the best thing here. I already mentioned one example of how to
do this: make the responsibility for 0 => -1 translation the job of
TarWALDumpReadPage() rather than astreamer_wal_read(). But I think
there might be a little more we can do. In particular, I wonder
whether we could say that astreamer_wal_read() is only responsible for
filling the buffer, and the caller, TarWALDumpReadPage() in this case,
needs to empty it. That seems like it might produce a cleaner
separation of duties.

Another thing that isn't so nice right now is that
verify_tar_archive() has to open and close the archive only for
init_tar_archive_reader() to be called to reopen it again just moments
later. It would be nicer to open the file just once and then keep it
open. Here again, I wonder if the separation of duties could be a bit
cleaner.

Is there a real need to pass XLogDumpPrivate to astreamer_wal_read or
astreamer_archive_read? The only things that they need are archive_fd,
archive_name, archive_streamer, archive_streamer_buf, and
archive_streamer_read_ptr. In other words, they really don't care
about any of the *existing* things that are in XLogDumpPrivate. This
makes me wonder whether we should actually try to make this new
astreamer completely independent of xlogreader. In other words,
instead of calling it astreamer_waldump() or astreamer_xlogreader() as
I proposed above, maybe it could be a completely generic astreamer,
say astreamer_stringinfo_new(StringInfo *buf) that just appends to the
buffer. That would require also moving the stuff out of
astreamer_wal_read() that knows about XLogRecPtr, but why does that
function need to know about XLogRecPtr? Couldn't the caller figure out
that part and just tell this function how many bytes are needed?

--
Robert Haas

---

From: Robert Haas <[email-masked]>
Date: Fri, 12 Sep 2025 16:27:07 -0400

> that part and just tell this function how many bytes are needed?

Hmm, on further thought, I think this was a silly idea. Part of the
intended function of this astreamer is to make sure we're only reading
WAL files from the archive, and eventually reordering them if
required, so obviously something completely generic isn't going to
work. Maybe there's a way to make this look a little cleaner and
tidier but this isn't it...

---

From: Amul Sul <[email-masked]>
Date: Thu, 25 Sep 2025 13:48:13 +0530

> directory.") -- maye bit a bit more verbose or use a message from

Done.

>     pg_waldump: error: could not open file "blah": Not a directory

Now, an error will be thrown if any additional command-line
arguments are provided when an archive is specified, similar to how
existing extra arguments are handled.

> misleading error message: could not read WAL data from "pg_wal.tar"

Now., added a much better error message for that case.

> and also having set TMPDIR to usually /tmp can be slow or too small.

Now, emitting info messages, but I'm not sure whether we should have
info or debug.

> mechanism preventing it from being used that way).

I am not sure what the worst-case scenario would be or what a good
alternative is.

> triggered with combo of options of nonsequential files in tar + wrong

Done.

> it to decode WAL directly` , but those pg_waldump are earlier patches,

Right, fixed.

> display anything related to verifing WALs, but it could)

If we decide to do that, it could be a separate project, IMHO.

> --end=LSN is made into such a future that it doesn't exist.

The behavior will be kept as if a directory was provided with a start
and end LSN.

Thanks again for the review. I'll post the new patches in my next reply.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Thu, 25 Sep 2025 13:54:51 +0530

> Here are some review comments on v3-0004:

Thanks for the review. My replies are below.

Done.

> invites confusion with the "content" callback.

Done -- renamed to astreamer_waldump_new().

Done.

> +       return (count - nbytes) ? (count - nbytes) : -1;

Agreed, that's a bit odd. This seems to be leftover code from the experimental
patch. The astreamer_wal_read() function should behave like WALRead():
it should either successfully read all the requested bytes or throw an
error. Corrected in the attached version.

> "readBuf". There are no existing uses of readBuff in the code base.

The existing WALDumpReadPage() function has a "readBuff" argument, and
I've used it that way for consistency.

> or equal to (or exactly equal to?) the LSN immediately following the

Updated the comment. The similar assertion exists right before
copying to the readBuff.

> open. Here again, I wonder if the separation of duties could be a bit

Prefer to keep those separate, assuming that reopening the file won't
cause any significant harm. Let me know if you think otherwise.

Attached the updated version, kindly have a look.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Mon, 29 Sep 2025 11:15:31 -0400

> cause any significant harm. Let me know if you think otherwise.

Well, I guess I'd like to know why we can't do better. I'm not really
worried about performance, but reopening the file means that you can
never make it work with reading from a pipe.

---

From: Amul Sul <[email-masked]>
Date: Mon, 29 Sep 2025 21:47:10 +0530

> never make it work with reading from a pipe.

I have some skepticism regarding the extra coding that might be
introduced, as performance is not my primary concern here. If we aim
to keep the file open only once, that logic should be implemented
before calling verify_tar_archive(), not inside it. Implementing the
open and close logic within verify_tar_archive() and
free_tar_archive_reader() would create a confusing and scattered
pattern, especially since these separate operations require only two
lines of code each (open and close if it's a tar file). My second,
concern is that after verify_tar_archive(), we might need to reset the
file reader offset to the beginning. While reusing the buffered data
from the first iteration is technically possible, that only works if
the desired start LSN is at the absolute beginning of the archive, or
later in the sequence, which cannot be reliably guaranteed. Therefore,
for simplicity and avoid the complexity of managing that offset reset
code, I am thinking of a simpler approach.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Fri, 10 Oct 2025 14:01:59 -0400

On Mon, Sep 29, 2025 at 12:17 PM Amul Sul <[email-masked]> wrote:
> While reusing the buffered data
> later in the sequence, which cannot be reliably guaranteed.

I spent a bunch of time studying this code today and I think that the
problem you're talking about here is evidence of a design problem with
astreamer_wal_read() and some of the other code in
astreamer_waldump.c. Your code calls astreamer_wal_read() when it
wants to peek at the first xlog block to determine the WAL segment
size, and it also calls astreamer_wal_read() when it wants read WAL
sequentially beginning at the start LSN and continuing until it
reaches the end LSN. However, these two cases have very different
requirements. verify_tar_archive(), which is misleadingly named and
really exists to determine the WAL segment size, just wants to read
the first xlog block that physically appears in the archive. Every
xlog block will have the same WAL segment size, so it does not matter
which one we read. On the other hand, TarWALDumpReadPage wants to read
WAL in sequential order. In other words, one call to
astreamer_wal_read() really wants to read a block without any block
reordering, and the other call wants to read a block with block
reordering.

To me, it looks like the problem here is that the block reordering
functionality should live on top of the astreamer, not inside of it.
Imagine that astreamer just spits out the bytes in the order in which
they physically appear in the archive, and then there's another
component that consumes and reorders those bytes. So, you read data
and push it into the astreamer until the number of bytes in the output
buffer is at least XLOG_BLCKSZ, and then from there you extract the
WAL segment size. Then, you call XLogReaderAllocate() and enter the
main loop. The reordering logic lives inside of TarWALDumpReadPage().
Each time it gets data from the astreamer's buffer, it either returns
it to the caller if it's in order or buffers it using temporary files
if not.

I found it's actually quite easy to write a patch that avoids
reopening the file. Here it is, on top of your v4:

diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 2c42df46d43..c4346a5e211 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -368,17 +368,8 @@ init_tar_archive_reader(XLogDumpPrivate *private,
const char *waldir,
                         XLogRecPtr startptr, XLogRecPtr endptr,
                         pg_compress_algorithm compression)
 {
-    int         fd;
     astreamer  *streamer;

-    /* Open tar archive and store its file descriptor */
-    fd = open_file_in_directory(waldir, private->archive_name);
-
-    if (fd < 0)
-        pg_fatal("could not open file \"%s\"", private->archive_name);
-
-    private->archive_fd = fd;
-
     /*
      * Create an appropriate chain of archive streamers for reading the given
      * tar archive.
@@ -1416,12 +1407,22 @@ main(int argc, char **argv)
     /* we have everything we need, start reading */
     if (is_tar)
     {
+        /* Open tar archive and store its file descriptor */
+        private.archive_fd =
+            open_file_in_directory(waldir, private.archive_name);
+        if (private.archive_fd < 0)
+            pg_fatal("could not open file \"%s\"", private.archive_name);
+
         /* Verify that the archive contains valid WAL files */
         waldir = waldir ? pg_strdup(waldir) : pg_strdup(".");
         init_tar_archive_reader(&private, waldir, InvalidXLogRecPtr,
                                 InvalidXLogRecPtr, compression);
         verify_tar_archive(&private);
-        free_tar_archive_reader(&private);
+        astreamer_free(private.archive_streamer);
+
+        if (lseek(private.archive_fd, 0, SEEK_SET) != 0)
+            pg_log_error("could not seek in file \"%s\": %m",
+                         private.archive_name);

         /* Set up for reading tar file */
         init_tar_archive_reader(&private, waldir, private.startptr,

Of course, this is not really what we want to do: it avoids reopening
the file, but because we can't back up the archive streamer once it's
been created, we have to lseek back to the beginning of the file. But
notice how silly this looks: with this patch, we free the archive
reader and immediately create a new archive reader that is exactly the
same in every way except that we call astreamer_waldump_new(startptr,
endptr, private) instead of astreamer_waldump_new(InvalidXLogRecPtr,
InvalidXLogRecPtr, private). We could arrange to update the original
archive streamer with new values of startSegNo and endSegNo after
verify_tar_archive(), but that's still not quite good enough, because
we might have already made some decisions on what to do with the data
that we read that it's too late to reverse. But, what that means is
that the astreamer_waldump machinery is not smart enough to read one
block of data without making irreversible decisions from which we
can't recover without recreating the entire object. I think we can,
and should, try to do better.

It's also worth noting that the unfortunate layering doesn't just
require us to read the first block of the file: it also complicates
the code in various places. The fact that astreamer_wal_read() needs a
special case for XLogRecPtrIsInvalid(recptr) is a direct result of
this problem, and the READ_ANY_WAL() macro and both the places that
test it are also direct results of this problem. In other words, I'm
arguing that astreamer_wal_read() is incorrectly defined, and that
error creates ugliness in the code both above and below
astreamer_wal_read().

While I'm on the topic of astreamer_wal_read(), here are a few other
problems I noticed:

* The return value is not documented, and it seems to always be count,
in which case it might as well return void. The caller already has the
value they passed for count.
* It seems like it would be more appropriate to assert that endPtr >=
len and just set startPtr = endPtr - len. I don't see how len > endPtr
can ever happen, and I bet bad things will happen if it does.
* "pg_waldump never ask the same" -> "pg_waldump never asks for the same"

Also, this is absolutely not OK with me:

                        /* Fetch more data */
                        if (astreamer_archive_read(privateInfo) == 0)
                        {
                                char            fname[MAXFNAMELEN];
                                XLogSegNo       segno;

                                XLByteToSeg(targetPagePtr, segno, WalSegSz); an
                                XLogFileName(fname,
privateInfo->timeline, segno, WalSegSz);

                                pg_fatal("could not find file \"%s\"
in \"%s\" archive",
                                                 fname,
privateInfo->archive_name);
                        }

astreamer_archive_read() will return 0 if we reach the end of the
tarfile, so this is saying that if we reach the end of the tar file
without finding the range of bytes for which we're looking, the
explanation must be that the relevant WAL file is missing from the
archive. But that is way too much action at a distance. I was able to
easily construct a counterexample by copying the first 81920 bytes of
a valid WAL file and then doing this:

[robert.haas pgsql-meson]$ tar tf pg_wal.tar
000000010000000000000005
[robert.haas pgsql-meson]$ pg_waldump -s 0/050008D8 -e 0/05FFED98
pg_wal.tar >/dev/null
pg_waldump: error: could not find file "000000010000000000000005" in
"pg_wal.tar" archive

Without the redirection to /dev/null, what happened was that
pg_waldump printed out a bunch of records from
000000010000000000000005 and then said that 000000010000000000000005
could not be found, which is obviously silly. But the fact that I
found a specific counterexample here isn't even really the point. The
point is that there's a big gap between what we actually know at this
point (which is that we've read the whole input file) and what the
message is claiming (which is that the reason must be that the file is
missing from the archive). Even if the counterexample above didn't
exist and that really were the only way for that to happen as of
today, that's very fragile. Maybe some future code change will make it
so that there's a second reason that could happen. How would somebody
realize that they had created a second condition by means of which
this code could be reached? If they did realize it, how would they get
the correct error to be reported?

I'm not quite sure how this should be fixed, but I strongly suspect
that the error report here needs to move closer to the code that is
doing the file reordering. Aside from the possibility of the file
being missing and the possibility of the file being too short, a third
possibility is that targetPagePtr retreats between one call and the
next. That really shouldn't happen, but there are no asserts here
verifying that it doesn't.

I also don't like the fact that one call to astreamer_archive_read()
checks the return value (but only whether it's zero, the specific
return value apparently doesn't matter, so why doesn't it return
bool?) and the other doesn't. That kind of coding pattern is very
rarely correct. The code says:

    /* Continue reading from the open WAL segment, if any */
    if (state->seg.ws_file >= 0)
    {
        /*
         * To prevent a race condition where the archive streamer is still
         * exporting a file that we are trying to read, we invoke the streamer
         * to ensure enough data is available.
         */
        if (private->curSegNo == state->seg.ws_segno)
            astreamer_archive_read(private);

        return WALDumpReadPage(state, targetPagePtr, reqLen, targetPtr,
                               readBuff);
    }

But it's unclear why this should be good enough to ensure that enough
data is available. astreamer_archive_read() might read zero bytes and
return 0, so this doesn't really guarantee anything at all. On the
other hand, even if astereamer_archive_read() returns a non-zero
value, it's only going to read READ_CHUNK_SIZE bytes from the
underlying file, so if more than that needs to be read in order for us
to have enough data, we won't. I think it's very hard to imagine a
situation in which you can call astreamer_archive_read() without using
some loop. That's what astreamer_wal_read() does: it calls
astreamer_archive_read() until it either returns 0 -- in which case we
know we've failed -- or until we have enough data. Here we just hope
that calling it once is enough, and that checking for errors is
unimportant. I also don't understand the reference to a race
condition, because there's only one process with one thread here, I
believe, so what would be racing against?

Another thing I noticed is that astreamer_archive_read() makes
reference to decrypting, but there's no cryptography involved in any
of this.

---

From: Amul Sul <[email-masked]>
Date: Thu, 16 Oct 2025 17:18:31 +0530

> it to the caller if it's in order or buffers it using temporary files

I initially considered implementing the reordering logic outside of
astreamer when we first discussed this project, but the implementation
could get complicated -- or at least feel hacky. Let me explain why:

astreamer reads the archive in fixed-size chunks (here it is 128KB).
Sometimes, a single read can contain data from two WAL files --
specifically, the tail end of one file and the start of the next --
because of how they’re physically stored in the archive. astreamer
knows where one file ends and another begins through tags like
ASTREAMER_MEMBER_HEADER, ASTREAMER_MEMBER_CONTENTS, and
ASTREAMER_MEMBER_TRAILER. However, it can’t pause mid-chunk to hold
data from the next file once the previous one ends and for the caller;
it pushes the entire chunk it has read to the target buffer.

So, if we put the reordering logic outside the streamer, we’d
sometimes be receiving buffers containing mixed data from two WAL
files. The caller would then need to correctly identify WAL file
boundaries within those buffers. This would require passing extra
metadata -- like segment numbers for the WAL files in the buffer, plus
start and end offsets of those segments within the buffer. While not
impossible, it feels a bit hacky and I'm unsure if that’s the best
approach.

> and should, try to do better.

Agreed.

> value they passed for count.

The caller will be xlogreader, and I believe we shouldn't change that.
For the same reason, WALDumpReadPage() also returns the same.

> * "pg_waldump never ask the same" -> "pg_waldump never asks for the same"

Ok.

> [robert.haas pgsql-meson]$ tar tf pg_wal.tar

Agreed, I'll think about this.

> some loop.

The loop isn't needed because the caller always requests 8KB of data,
while READ_CHUNK_SIZE is 128KB. It’s assumed that the astreamer has
already created the file with some initial data. For example, if only
a few bytes have been written so far, when we reach
TarWALDumpReadPage(), it detects that we’re reading the same file
that the astreamer is still writing to and hasn’t finished. It then request to
appends 128KB of data by calling astreamer_archive_read, even though we
only need 8KB at a time. This process repeats each time the next 8KBchunk is
requested: astreamer_archive_read() appends another 128KB,and continues until
the file has been fully read and written.

> That's what astreamer_wal_read() does: it calls

In the case where the astreamer is exporting a file to disk but hasn’t
finished writing it, and we call TarWALDumpReadPage() to request
block(s) from that WAL file, we can read only up to the existing
blocks in the file. Since the file is incomplete, reading may fail
later. To handle this, astreamer_archive_read() is invoked to append
more data -- usually more than the requested amount, as explained
earlier. That is the race condition I am trying to handle.

Now, regarding the concern of astreamer_archive_read() returning zero
without reading or appending any data: this can happen only if the WAL
is shorter than expected -- an incomplete. In that case,
WALDumpReadPage() will raise the appropriate error, we don't have to
check at that point, I think.

> reference to decrypting, but there's no cryptography involved in any

I think that was a typo -- I meant decompression.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Mon, 20 Oct 2025 10:34:50 -0400

> it pushes the entire chunk it has read to the target buffer.

Right, this makes sense.

> impossible, it feels a bit hacky and I'm unsure if that’s the best

I agree that we need that kind of metadata, but I don't see why our
need for it depends on where we do the reordering. That is, if we do
the reordering above the astreamer layer, we need to keep track of the
origin of each chunk of WAL bytes, and if we do the reordering within
the astreamer layer, we still need to keep track of the origin of the
WAL bytes. Doing the ordering properly requires that tracking, but it
doesn't say anything about where that tracking has to be performed.

I think it might be better if we didn't write to the astreamer's
buffer at all. For example, suppose we create a struct that looks
approximately like this:

struct ChunkOfDecodedWAL
{
     XLogSegNo segno; // could also be XLogRecPtr start_lsn or char
*walfilename or whatever
     StringInfoData buffer;
     char *spillfilename; // or whatever we use to identify the temporary files
     bool already_removed;
     // potentially other metadata
};

Then, create a hash table and key it on the segno whatever. Have the
astreamer write to the hash table: when it gets a chunk of WAL, it
looks up or creates the relevant hash table entry and appends the data
to the buffer. At any convenient point in the code, you can decide to
write the data from the buffer to a spill file, after which you
resetStringInfo() on the buffer and populate the spill file name. When
you've used up the data, you remove the spill file and set the
already_removed flag.

I think this could also help with the error reporting stuff. When you
get to the end of the file, you'll know all the files you saw and how
much data you read from each of them. So you could possibly do
something like

ERROR: LSN %08X/%08X not found in archive "\%s\"
DETAIL: WAL segment %s is not present in the archive
-or
DETAIL: WAL segment %s was expected to be %u bytes, but was only %u bytes
-or-
DETAIL: whatever else can go wrong

The point is that every file you've ever seen has a hash table entry,
and in that hash table entry you can store everything about that file
that you need to know, whether that's the file data, the disk file
that contains the file data, the fact that we already threw the data
away, or any other fact that you can imagine wanting to know.

Said differently, the astreamer buffer is not really a great place to
write data. It exists because when we're just forwarding data from one
astreamer to the next, we will often need to buffer a small amount of
data to avoid terrible performance. However, it's only there to be
used when we don't have something better. I don't think any astreamer
that is intended to be the last one in the chain currently writes to
the buffer -- they write to the output file, or whatever, because
using an in-memory buffer as your final output destination is not a
real good plan.

> For the same reason, WALDumpReadPage() also returns the same.

OK, but then you can make that clear via a brief comment.

> the file has been fully read and written.

Sure, but you don't know how much data is going to come out the other
end of the astreamer pipeline. Since the data is (possibly)
compressed, you expect at least as many bytes to emerge from the
output end as you add to the input end, but it's not a good idea to
rely on assumptions like that. Sometimes compressors end up making the
data slightly larger instead of smaller. It's unlikely that the effect
would be so dramatic that adding 128kB to one end of the pipeline
would make less than 8kB emerge from the other end, but it's not a
good idea to rely on assumptions like that. Not that this is a real
thing, but imagine that the compressed file had something in the
middle of it that behaved like a comment in C code, i.e. it didn't
generate any output.

> earlier. That is the race condition I am trying to handle.

That's not what a race condition is:
https://en.wikipedia.org/wiki/Race_condition

> check at that point, I think.

I'm not going to accept that kind of justification -- it is too
fragile to assume that you don't need to check for an error because it
"can't happen". Sometimes that is reasonable, but there is quite a lot
of action-at-a-distance here, so it does not feel safe.

> I think that was a typo -- I meant decompression.

I figured as much, but it still needs fixing.

---

From: Amul Sul <[email-masked]>
Date: Thu, 6 Nov 2025 14:33:05 +0530

> using an in-memory buffer as your final output destination is not a

Make sense, I implemented this approach in the attached version, but
with a different structure name and a slightly different error
message. In the error output using the WAL file name instead of the
LSN. This is because the LSN at that point may differ from the
user-provided one (it might have been adjusted to the start of a WAL
page by xlogreader). This follows the same style used in the routine
that reads the WAL file. The LSN values (user provided) are only used
in error messages generated at the very beginning, specifically in the
main() function of pg_waldump.

I have also restructured the code by moving most of the tar file
reading logic out of pg_waldump.c into astreamer_waldump.c, which has
now been renamed to archive_waldump.c.

Kindly have a look at the attached version. Thank you !

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Mon, 17 Nov 2025 10:20:48 +0530

> Kindly have a look at the attached version. Thank you !

Attached is the rebased version against the latest master head (e76defbcf09).

Regards,
Amul

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 19 Nov 2025 09:20:14 +0100

> Attached is the rebased version against the latest master head (e76defbcf09).

Hi Amul, thanks for working on this. I haven't really looked at the
source code deeply (I trust Robert eyes much more than mine on this
one), just skimmed a little bit:

1. As stated earlier, get_tmp_walseg_path() is still vulnerable (it
uses predictable path that could be used by attacker in $TMPDIR)

2. On the usability front:

a. If you do `pg_waldump --path pg_wal.tar -s 0/31000000` it will dump
a lot of WAL records and then print final:
pg_waldump: error: could not find file "000000010000000000000034" in archive

However, with `pg_waldump --path pg_wal.tar -s 0/31000000
--stats=record` (not passing '-e') it will simply bailout without
printing stats and with error:
pg_waldump: error: could not find file "000000010000000000000034" in archive

IMHO, it could print stats if it was capable of getting at least 1 WAL record.

3. The most critical issue for me was the initial lack of error
pass-through from pg_waldump (when used with WALs in tar) to the
pg_verifybackup. Now it works fine, so thanks for this:

a. pg_waldump is capable of discovering missing WALs as requested and
throwing proper return code (good)
$ /usr/pgsql19/bin/pg_waldump --path pg_wal.tar -s 0/31005F70 -e 0/343D2650 -q
pg_waldump: error: could not find file "000000010000000000000034" in archive
$ echo $?
1
$

b. pg_verifybackup now also complains properly with missing WAL inside tar

$ tar --delete -f pg_wal.tar 000000010000000000000032 # simulate loss of file
$ tar -tf pg_wal.tar
000000010000000000000031
archive_status/000000010000000000000031.done
archive_status/000000010000000000000032.done
000000010000000000000033
$ grep Start-LSN backup_manifest
{ "Timeline": 1, "Start-LSN": "0/31005F70", "End-LSN": "0/333D2650" }
$ /usr/pgsql19/bin/pg_verifybackup -P /tmp/basebackup/
791372/791372 kB (100%) verified
pg_waldump: error: could not find file "000000010000000000000032" in archive
pg_verifybackup: error: WAL parsing failed for timeline 1
$ echo $?
1
$

-J.

---

From: Amul Sul <[email-masked]>
Date: Fri, 21 Nov 2025 17:14:26 +0530

> uses predictable path that could be used by attacker in $TMPDIR)

Yeah, I haven't done anything regarding this since I am unsure of what
should be done and what the risks involved are. I am thinking of
taking Robert's opinion on this.

> IMHO, it could print stats if it was capable of getting at least 1 WAL record.

The similar behavior in the current pg_waldump when using the --path
option with a WAL directory and a starting LSN. E.g:

$ pg_waldump -s 0/04FE36E0 --path=/tmp/backup/tmp/ --stats=record

pg_waldump: first record is after 0/04FE36E0, at 0/04FE3F90, skipping
over 2224 bytes
pg_waldump: error: could not find file "000000010000000000000009": No
such file or directory

> pg_verifybackup. Now it works fine, so thanks for this:

Thanks, that was exactly the intention -- to complete pg_verifybackup
for tar-formatted backup verification.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Fri, 21 Nov 2025 17:46:24 +0530

> > Kindly have a look at the attached version. Thank you !

Attached is the updated version. I have fixed an assertion failure
that can occasionally occur with a partial WAL page read.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Tue, 25 Nov 2025 12:07:32 +0530

> taking Robert's opinion on this.

Per offline discussion with Robert and Jakub, I have updated the patch
to use mkdtemp() as suggested, which is already available in the tree
for similar purposes. Thanks !

Regards,
Amul

---

From: Chao Li <[email-masked]>
Date: Tue, 25 Nov 2025 16:50:29 +0800

Hi Amul,

I reviewed the patch and got some comments:

> On Nov 25, 2025, at 14:37, Amul Sul <[email-masked]> wrote:
> 
> 
> Regards,
> Amul
> <v8-0001-Refactor-pg_waldump-Move-some-declarations-to-new.patch><v8-0002-Refactor-pg_waldump-Separate-logic-used-to-calcul.patch><v8-0003-Refactor-pg_waldump-Restructure-TAP-tests.patch><v8-0004-pg_waldump-Add-support-for-archived-WAL-decoding.patch><v8-0005-pg_waldump-Remove-the-restriction-on-the-order-of.patch><v8-0006-pg_verifybackup-Delay-default-WAL-directory-prepa.patch><v8-0007-pg_verifybackup-Rename-the-wal-directory-switch-t.patch><v8-0008-pg_verifybackup-enabled-WAL-parsing-for-tar-forma.patch>

1 - 0001 - pg_waldump.h
```
+ * pg_waldump.h - decode and display WAL
+ *
+ * Copyright (c) 2013-2025, PostgreSQL Global Development Group
```

This header file is brand new, so copyright year should be only 2025.

2 - 0001 - pg_waldump.c
```
-static int	WalSegSz;
+int			WalSegSz = DEFAULT_XLOG_SEG_SIZE;
```

0001 claims a refactoring, but if you initialize WalSegSz with DEFAULT_XLOG_SEG_SIZE, then the behavior is changing, this change is no longer a pure refactor.

I would suggest leave WalSegSz uninitiated (compiler will set 0 to it), then no behavior change, so that 0001 stays a self-contained pure refactor.

The other nit thing is that, as “static” is removed, now “WalSegSz” is placed in middle of two static variables, which looks not good. If I were making the code change, I would have moved WalSegSz to after all static variables.

3 - 0002
```
@@ -383,21 +406,11 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 				XLogRecPtr targetPtr, char *readBuff)
 {
 	XLogDumpPrivate *private = state->private_data;
-	int			count = XLOG_BLCKSZ;
+	int			count = required_read_len(private, targetPagePtr, reqLen);
 	WALReadError errinfo;
 
-	if (XLogRecPtrIsValid(private->endptr))
-	{
-		if (targetPagePtr + XLOG_BLCKSZ <= private->endptr)
-			count = XLOG_BLCKSZ;
-		else if (targetPagePtr + reqLen <= private->endptr)
-			count = private->endptr - targetPagePtr;
-		else
-		{
-			private->endptr_reached = true;
-			return -1;
-		}
-	}
+	if (private->endptr_reached)
+		return -1;
```

This change introduces a logic hole. In old code, it sets private->endptr_reached = true; and return -1. In the code code, count and private->endptr_reached assignments are wrapped into required_read_len(). However, required_read_len() doesn’t check if private->endptr_reached has already been true, so that the logic hole is that, if private->endptr_reached is already true when calling required_read_len(), and required_read_len() returns a positive count, if (private->endptr_reached) will also be satisfied and return -1 from the function.

So, to be safe, we should check “if (count < 0) return -1”.

4 - 0002
```
+/* Returns the size in bytes of the data to be read. */
+static inline int
+required_read_len(XLogDumpPrivate *private, XLogRecPtr targetPagePtr,
+				  int reqLen)
+{
```

The function comment is too simple. It doesn’t cover the case where -1 is returned.

5 - 0003
```
+my @scenario = (
+	{
+		'path' => $node->data_dir
+	});
 
-@lines = test_pg_waldump('--limit' => 6);
-is(@lines, 6, 'limit option observed');
+for my $scenario (@scenario)
+{
```

"my @scenario” should be "my @scenarios”, so that for line become "for my $scenario (@scenarios)”, a little bit clearer.

6 - 0003
```
+	SKIP:
+	{
```

Why SKIP label is defined here? A SKIP label usually follows a skip statement, for example: in bin/pg_ctl/t/001_start_stop.pl
```
SKIP:
{
skip "unix-style permissions not supported on Windows", 2
if ($windows_os);

ok(-f $logFileName);
ok(check_mode_recursive("$tempdir/data", 0700, 0600));
}
```

7 - 0004 - Makefile
```
 	$(WIN32RES) \
 	compat.o \
 	pg_waldump.o \
+	archive_waldump.o \
 	rmgrdesc.o \
 	xlogreader.o \
 	xlogstats.o
```

Obviously the list was in alphabetical order, so archive_waldump.o should be placed before compat.o.

8 - 0004
```
+/*
+ * pg_waldump's XLogReaderRoutine->page_read callback to support dumping WAL
+ * files from tar archives.
+ */
+static int
+TarWALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
+				   XLogRecPtr targetPtr, char *readBuff)
+{
+	XLogDumpPrivate *private = state->private_data;
+	int			count = required_read_len(private, targetPagePtr, reqLen);
```

Looking the page_read’s spec:
```
	/*
	 * Data input callback
	 *
	 * This callback shall read at least reqLen valid bytes of the xlog page
	 * starting at targetPagePtr, and store them in readBuf.  The callback
	 * shall return the number of bytes read (never more than XLOG_BLCKSZ), or
	 * -1 on failure.  The callback shall sleep, if necessary, to wait for the
	 * requested bytes to become available.  The callback will not be invoked
	 * again for the same page unless more than the returned number of bytes
	 * are needed.
	 *
	 * targetRecPtr is the position of the WAL record we're reading.  Usually
	 * it is equal to targetPagePtr + reqLen, but sometimes xlogreader needs
	 * to read and verify the page or segment header, before it reads the
	 * actual WAL record it's interested in.  In that case, targetRecPtr can
	 * be used to determine which timeline to read the page from.
	 *
	 * The callback shall set ->seg.ws_tli to the TLI of the file the page was
	 * read from.
	 */
	XLogPageReadCB page_read;
```

It says that page_read must read reqLen bytes, otherwise it should wait for more bytes.

However, TarWALDumpReadPage just calculate how many bytes can read and only read that long, which breaks the protocol. Is it a problem?

9 - 0004
```
+/*
+ * Create an astreamer that can read WAL from tar file.
+ */
+static astreamer *
+astreamer_waldump_new(XLogDumpPrivate *privateInfo)
+{
+	astreamer_waldump *streamer;
+
+	streamer = palloc0(sizeof(astreamer_waldump));
+	*((const astreamer_ops **) &streamer->base.bbs_ops) =
+		&astreamer_waldump_ops;
+
+	streamer->privateInfo = privateInfo;
+
+	return &streamer->base;
+}
```

This function allocates memory for streamer but only returns &streamer->base, so memory of streamer is leaked.

Also, in the function comment, “from tar file” => “from a tar file”.

10 - 0004
```
+ * End-of-stream processing for a astreamer_waldump stream.
```

Nit typo: a => an

11 - 0004
```
+	if (!IsValidWalSegSize(WalSegSz))
+	{
+		pg_log_error(ngettext("invalid WAL segment size in WAL file from archive \"%s\" (%d byte)",
+							  "invalid WAL segment size in WAL file from archive \"%s\" (%d bytes)",
+							  WalSegSz),
+					 privateInfo->archive_name, WalSegSz);
+		pg_log_error_detail("The WAL segment size must be a power of two between 1 MB and 1 GB.");
+		exit(1);
+	}
```

Why don’t pg_fatal()?

12 - 0005
```
+		/* Create a temporary file if one does not already exist */
+		if (!entry->tmpseg_exists)
+		{
+			write_fp = prepare_tmp_write(entry->segno);
+			entry->tmpseg_exists = true;
+		}
+
+		/* Flush data from the buffer to the file */
+		perform_tmp_write(entry->segno, &entry->buf, write_fp);
+		resetStringInfo(&entry->buf);
+
+		/*
+		 * The change in the current segment entry indicates that the reading
+		 * of this file has ended.
+		 */
+		if (entry != privateInfo->cur_wal && write_fp != NULL)
+		{
+			fclose(write_fp);
+			write_fp = NULL;
+		}
```

When entry->tmpseg_exists is true, then write_fp will not be initialized, but there should be a check to make sure write_fp is not NULL before perform_tmp_write(). 

Also, if write_fp != NULL, should we anyway close the file without considering entry != privateInfo->cur_wal? Otherwise write_fp may be left open.

13 - 0005
```
+	 * Use the directory specified by the TEMDIR environment variable. If it’s
```

Typo: TEMDIR => TMPDIR

14 - 0005
```
+ * Set up a temporary directory to temporarily store WAL segments.
```

temporary and temporarily are redundant.

No comment for 0007.

15 - 0007

I wonder why we need to manually po files? This is the first time I see a patch including po file changes.

16 - 0008
```
+		{
+			pg_log_error("wal archive not found");
+			pg_log_error_hint("Specify the correct path using the option -w/--wal-path."
+							  "Or you must use -n/--no-parse-wal when verifying a tar-format backup.");
+			exit(1);
+		}
```

“wal” should be “WAL”.

In the hint message, there should be a white space between the two sentences.

Again, why not pg_fatal().

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Amul Sul <[email-masked]>
Date: Wed, 26 Nov 2025 11:32:04 +0530

> I reviewed the patch and got some comments:

Thanks for the review. Replying inline below.

> This header file is brand new, so copyright year should be only 2025.

Fixed in the attached version.

Agreed.

> The other nit thing is that, as “static” is removed, now “WalSegSz” is placed in middle of two static variables, which looks not good. If I were making the code change, I would have moved WalSegSz…

I placed it before the static declaration.

> So, to be safe, we should check “if (count < 0) return -1”.

I do not really understand the logical hole where the behaviour is the
same as the previous, but I like the idea of checking endptr_reached.
This is quite unlikely to be true, but it looks like good practice to
check that flag before setting it. Did it that way in the attached
version.

> The function comment is too simple. It doesn’t cover the case where -1 is returned.

Okay.

Done.

> ok(check_mode_recursive("$tempdir/data", 0700, 0600));

Yeah, I knew that, but that is needed in the next patch where I wanted
to avoid a large diff when introducing SKIP and the associated
indentation. This patch is not expected to be committed independently,
and I have added a note in the commit message for the same.

Done.

> However,
 just calculate how many bytes can read and only read that long, which
breaks the protocol. Is it a problem?

The behaviour is the same as the routine used to read the bare WAL
file. I don't think there will be any problem for the pg_waldump.

> This function allocates memory for streamer but only returns &streamer->base, so memory of streamer is leaked.

May I know why you think there would be a memory leak? I believe the
address of the structure is the same as the address of its first
member, base. I am returning base because the goal is to return a
generic astreamer type, which is the standard approach used in other
archive streamer code.

Done.

> +               pg_log_error_detail("The WAL segment size must be a power of two between 1 MB and 1 GB.");

This is how we do when we need to emit error details as well.

> When entry->tmpseg_exists is true, then write_fp will not be initialized, but there should be a check to make sure write_fp is not NULL before perform_tmp_write().

perform_tmp_write() has assert for the same.

> Also, if write_fp != NULL, should we anyway close the file without considering entry != privateInfo->cur_wal? Otherwise write_fp may be left open.

We read the WAL from the tar file in chunks, and those same chunks are
written to the temporary file within the loop. If we close the
temporary file now, we will have to open it again later for the next
chunk write. Could you elaborate on a scenario where you believe this
file might be left open unintentionally?

Done.

> temporary and temporarily are redundant.

I believe that is grammatically correct and clear.

> I wonder why we need to manually po files? This is the first time I see a patch including po file changes.

Okay, I included that initially to ensure the PO file update wasn't
overlooked during commit. I have removed it to minimize the diff and
added the note in the patch commit message.

Done.

Thanks again for your review comments; they are quite helpful. Kindly
take a look at the attached version.

Regards,
Amul

---

From: Chao Li <[email-masked]>
Date: Wed, 26 Nov 2025 15:23:39 +0800

> On Nov 26, 2025, at 14:02, Amul Sul <[email-masked]> wrote:
> 
>> 9 - 0004
>> ```
>> +/*

Ah… Got it.

Best regards,

---

From: Amul Sul <[email-masked]>
Date: Tue, 10 Feb 2026 15:06:12 +0530

On Wed, Feb 4, 2026 at 6:39 PM Amul Sul <[email-masked]> wrote:
>
> On Wed, Jan 28, 2026 at 2:41 AM Robert Haas <[email-masked]> wrote:
> >
> > On Tue, Jan 27, 2026 at 7:07 AM Amul Sul <[email-masked]> wrote:
> > > In the attached version, I am using the WAL segment name as the hash
> > > key, which is much more straightforward. I have rewritten
> > > read_archive_wal_page(), and it looks much cleaner than before. The
> > > logic to discard irrelevant WAL files is still within
> > > get_archive_wal_entry. I added an explanation for setting cur_wal to
> > > NULL, which is now handled in the separate function I mentioned
> > > previously.
> > >
> > > Kindly have a look at the attached version; let me know if you are
> > > still not happy with the current approach for filtering/discarding
> > > irrelevant WAL segments. It isn't much different from the previous
> > > version, but I have tried to keep it in a separate routine for better
> > > code readability, with comments to make it easier to understand. I
> > > also added a comment for ArchivedWALFile.
> >
> > I feel like the division of labor between get_archive_wal_entry() and
> > read_archive_wal_page() is odd. I noticed this in the last version,
> > too, and it still seems to be the case. get_archive_wal_entry() first
> > calls ArchivedWAL_lookup(). If that finds an entry, it just returns.
> > If it doesn't, it loops until an entry for the requested file shows up
> > and then returns it. Then control returns to read_archive_wal_page()
> > which loops some more until we have all the data we need for the
> > requested file. But it seems odd to me to have two separate loops
> > here. I think that the first loop is going to call read_archive_file()
> > until we find the beginning of the file that we care about and then
> > the second one is going to call read_archive_file() some more until we
> > have read enough of it to satisfy the request. It feels odd to me to
> > do it that way, as if we told somebody to first wait until 9 o'clock
> > and then wait another 30 minutes, instead of just telling them to wait
> > until 9:30. I realize it's not quite the same thing, because apart
> > from calling read_archive_file(), the two loops do different things,
> > but I still think it looks odd.
> >
> > + /*
> > + * Ignore if the timeline is different or the current segment is not
> > + * the desired one.
> > + */
> > + XLogFromFileName(entry->fname, &curSegTimeline, &curSegNo, WalSegSz);
> > + if (privateInfo->timeline != curSegTimeline ||
> > + privateInfo->startSegNo > curSegNo ||
> > + privateInfo->endSegNo < curSegNo ||
> > + segno > curSegNo)
> > + {
> > + free_archive_wal_entry(entry->fname, privateInfo);
> > + continue;
> > + }
> >
> > The comment doesn't match the code. If it did, the test would be
> > (privateInfo->timeline != curSegTimeline || segno != curSegno). But
> > instead the segno test is > rather than !=, and the checks against
> > startSegNo and endSegNo aren't explained at all. I think I understand
> > why the segno test uses > rather than !=, but it's the point of the
> > comment to explain things like that, rather than leaving the reader to
> > guess. And I don't know why we also need to test startSegNo and
> > endSegNo.
> >
> > I also wonder what the point is of doing XLogFromFileName() on the
> > fname provided by the caller and then again on entry->fname. Couldn't
> > you just compare the strings?
> >
> > Again, the division of labor is really odd here. It's the job of
> > astreamer_waldump_content() to skip things that aren't WAL files at
> > all, but it's the job of get_archive_wal_entry() to skip things that
> > are WAL files but not the one we want. I disagree with putting those
> > checks in completely separate parts of the code.
> >
>
> Keeping the timeline and segment start-end range checks inside the
> archive streamer creates a circular dependency that cannot be resolved
> without a 'dirty hack'. We must read the first available WAL file page
> to determine the wal_segment_size before it can calculate the target
> segment range. Moving the checks inside the streamer would make it
> impossible to process that initial file, as the necessary filtering
> parameters --  would still be unknown which would need to be skipped
> for the first read somehow. What if later we realized that the first
> WAL file which was allowed to be streamed by skipping that check is
> irrelevant and doesn't fall under the start-end segment range?
>

Please have a look at the attached version, specifically patch 0005.
In astreamer_waldump_content(), I have moved the WAL file filtration
check from get_archive_wal_entry(). This check will be skipped during
the initial read in init_archive_reader(), which instead performs it
explicitly once it determines the WAL segment size and the start/end
segments.

To access the WAL segment size inside astreamer_waldump_content(), I
have moved the WAL segment size variable into the XLogDumpPrivate
structure in the separate 0004 patch.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Wed, 18 Feb 2026 12:28:27 +0530

> structure in the separate 0004 patch.

Attached is an updated version including the aforesaid changes. It
includes a new refactoring patch (0001) that moves the logic for
identifying tar archives and their compression types from
pg_basebackup and pg_verifybackup into a separate-reusable function,
per a suggestion from Euler [1].  Additionally, I have added a test
for the contrecord decoding to the main patch (now 0006).

1] http://postgr.es/m/[email-masked]

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Mon, 2 Mar 2026 18:30:24 +0530

> 1] http://postgr.es/m/[email-masked]

Rebased against the latest master, fixed typos in code comments, and
replaced palloc0 with palloc0_object.

Regards,
Amul

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 3 Mar 2026 19:37:48 -0500

> replaced palloc0 with palloc0_object.

Hi Amul.

I think this looks in pretty good shape.

Attached are patches for a few things I think could be fixed. They are 
mostly self-explanatory. The TAP test fix is the only sane way I could 
come up with stopping the skip code you had from reporting a wildly 
inaccurate number of tests skipped. The sane way to do this from a 
Test::More perspective is a subtest, but unfortunately meson does not 
like subtest output, which is why we don't use it elsewhere, so the only 
way I could come up with was to split this out into a separate test. Of 
course, we might just say we don't care about the misreport, in which 
case we could just live with things as they are.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Tue, 3 Mar 2026 00:00:00 +0000
Subject: [PATCH] Add pg_verifybackup test for tar-format WAL verification

The new tar-format WAL verification in pg_verifybackup had no test
coverage for the case where pg_basebackup produces a separate
pg_wal.tar (--format=tar --wal-method=stream).  Add a test that takes
a tar-format backup and verifies it.
---
 src/bin/pg_verifybackup/t/007_wal.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/bin/pg_verifybackup/t/007_wal.pl b/src/bin/pg_verifybackup/t/007_wal.pl
index 8ad2234453d..0e0377bfacc 100644
--- a/src/bin/pg_verifybackup/t/007_wal.pl
+++ b/src/bin/pg_verifybackup/t/007_wal.pl
@@ -90,4 +90,20 @@ command_ok(
 	[ 'pg_verifybackup', $backup_path2 ],
 	'valid base backup with timeline > 1');
 
+# Test WAL verification for a tar-format backup with a separate pg_wal.tar,
+# as produced by pg_basebackup --format=tar --wal-method=stream.
+my $backup_path3 = $primary->backup_dir . '/test_tar_wal';
+$primary->command_ok(
+	[
+		'pg_basebackup',
+		'--pgdata' => $backup_path3,
+		'--no-sync',
+		'--format' => 'tar',
+		'--checkpoint' => 'fast'
+	],
+	"tar backup with separate pg_wal.tar");
+command_ok(
+	[ 'pg_verifybackup', $backup_path3 ],
+	'WAL verification succeeds with separate pg_wal.tar');
+
 done_testing();
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Tue, 3 Mar 2026 00:00:00 +0000
Subject: [PATCH] Split pg_waldump TAP tests into directory and archive files

The original 001_basic.pl mixed directory and tar archive tests in a
single SKIP loop with a hardcoded skip count of 3, but each scenario
actually runs ~19 assertions.  When tar is unavailable the skip count
was wrong, and the directory scenario was also wrongly guarded by the
tar-availability check.

Move all archive-related tests (tar, tar.gz) into a new
003_archive.pl that uses plan skip_all when tar is unavailable,
cleanly skipping the entire file.  001_basic.pl retains only
directory-based tests with no SKIP blocks needed.
---
 src/bin/pg_waldump/meson.build       |   1 +
 src/bin/pg_waldump/t/001_basic.pl    | 221 ++++++++++-----------------
 src/bin/pg_waldump/t/003_archive.pl  | 320 +++++++++++++++++++++++++++++++++++
 3 files changed, 396 insertions(+), 146 deletions(-)
 create mode 100644 src/bin/pg_waldump/t/003_archive.pl

diff --git a/src/bin/pg_waldump/meson.build b/src/bin/pg_waldump/meson.build
index 5296f21b82c..d2b4bd0c048 100644
--- a/src/bin/pg_waldump/meson.build
+++ b/src/bin/pg_waldump/meson.build
@@ -34,6 +34,7 @@ tests += {
     'tests': [
       't/001_basic.pl',
       't/002_save_fullpage.pl',
+      't/003_archive.pl',
     ],
   },
 }
diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index 9854c939007..282c9a37221 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -3,13 +3,9 @@
 
 use strict;
 use warnings FATAL => 'all';
-use Cwd;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
-use List::Util qw(shuffle);
-
-my $tar = $ENV{TAR};
 
 program_help_ok('pg_waldump');
 program_version_ok('pg_waldump');
@@ -195,8 +191,8 @@ END
 $$;
 });
 
-my $contrecord_lsn = $node->safe_psql('postgres',
-	'SELECT pg_current_wal_insert_lsn()');
+my $contrecord_lsn =
+  $node->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn()');
 # Generate contrecord record
 $node->safe_psql('postgres',
 	qq{SELECT pg_logical_emit_message(true, 'test 026', repeat('xyzxz', 123456))}
@@ -299,145 +295,78 @@ sub test_pg_waldump
 	return @lines;
 }
 
-# Create a tar archive, sorting the file order
-sub generate_archive
-{
-	my ($archive, $directory, $compression_flags) = @_;
-
-	my @files;
-	opendir my $dh, $directory or die "opendir: $!";
-	while (my $entry = readdir $dh) {
-		# Skip '.' and '..'
-		next if $entry eq '.' || $entry eq '..';
-		push @files, $entry;
-	}
-	closedir $dh;
-
-	@files = shuffle @files;
-
-	# move into the WAL directory before archiving files
-	my $cwd = getcwd;
-	chdir($directory) || die "chdir: $!";
-	command_ok([$tar, $compression_flags, $archive, @files]);
-	chdir($cwd) || die "chdir: $!";
-}
-
-my $tmp_dir = PostgreSQL::Test::Utils::tempdir_short();
-
-my @scenarios = (
-	{
-		'path' => $node->data_dir,
-		'is_archive' => 0,
-		'enabled' => 1
-	},
-	{
-		'path' => "$tmp_dir/pg_wal.tar",
-		'compression_method' => 'none',
-		'compression_flags' => '-cf',
-		'is_archive' => 1,
-		'enabled' => 1
-	},
-	{
-		'path' => "$tmp_dir/pg_wal.tar.gz",
-		'compression_method' => 'gzip',
-		'compression_flags' => '-czf',
-		'is_archive' => 1,
-		'enabled' => check_pg_config("#define HAVE_LIBZ 1")
-	});
-
-for my $scenario (@scenarios)
-{
-	my $path = $scenario->{'path'};
-
-	SKIP:
-	{
-		skip "tar command is not available", 3
-		  if !defined $tar;
-		skip "$scenario->{'compression_method'} compression not supported by this build", 3
-		  if !$scenario->{'enabled'} && $scenario->{'is_archive'};
-
-		  # create pg_wal archive
-		  if ($scenario->{'is_archive'})
-		  {
-			  generate_archive($path,
-				  $node->data_dir . '/pg_wal',
-				  $scenario->{'compression_flags'});
-		  }
-
-		command_fails_like(
-			[ 'pg_waldump', '--path' => $path ],
-			qr/error: no start WAL location given/,
-			'path option requires start location');
-		command_like(
-			[
-				'pg_waldump',
-				'--path' => $path,
-				'--start' => $start_lsn,
-				'--end' => $end_lsn,
-			],
-			qr/./,
-			'runs with path option and start and end locations');
-		command_fails_like(
-			[
-				'pg_waldump',
-				'--path' => $path,
-				'--start' => $start_lsn,
-			],
-			qr/error: error in WAL record at/,
-			'falling off the end of the WAL results in an error');
-
-		command_fails_like(
-			[
-				'pg_waldump', '--quiet',
-				'--path' => $path,
-				'--start' => $start_lsn
-			],
-			qr/error: error in WAL record at/,
-			'errors are shown with --quiet');
-
-		test_pg_waldump_skip_bytes($path, $start_lsn, $end_lsn);
-
-		my @lines = test_pg_waldump($path, $start_lsn, $end_lsn);
-		is(grep(!/^rmgr: \w/, @lines), 0, 'all output lines are rmgr lines');
-
-		@lines = test_pg_waldump($path, $contrecord_lsn, $end_lsn);
-		is(grep(!/^rmgr: \w/, @lines), 0, 'all output lines are rmgr lines');
-
-		test_pg_waldump_skip_bytes($path, $contrecord_lsn, $end_lsn);
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--limit' => 6);
-		is(@lines, 6, 'limit option observed');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--fullpage');
-		is(grep(!/^rmgr:.*\bFPW\b/, @lines), 0, 'all output lines are FPW');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--stats');
-		like($lines[0], qr/WAL statistics/, "statistics on stdout");
-		is(grep(/^rmgr:/, @lines), 0, 'no rmgr lines output');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--stats=record');
-		like($lines[0], qr/WAL statistics/, "statistics on stdout");
-		is(grep(/^rmgr:/, @lines), 0, 'no rmgr lines output');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--rmgr' => 'Btree');
-		is(grep(!/^rmgr: Btree/, @lines), 0, 'only Btree lines');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--fork' => 'init');
-		is(grep(!/fork init/, @lines), 0, 'only init fork lines');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn,
-			'--relation' => "$default_ts_oid/$postgres_db_oid/$rel_t1_oid");
-		is(grep(!/rel $default_ts_oid\/$postgres_db_oid\/$rel_t1_oid/, @lines),
-			0, 'only lines for selected relation');
-
-		@lines = test_pg_waldump($path, $start_lsn, $end_lsn,
-			'--relation' => "$default_ts_oid/$postgres_db_oid/$rel_i1a_oid",
-			'--block' => 1);
-		is(grep(!/\bblk 1\b/, @lines), 0, 'only lines for selected block');
-
-		# Cleanup.
-		unlink $path if $scenario->{'is_archive'};
-	}
-}
+my $path = $node->data_dir;
+
+command_fails_like(
+	[ 'pg_waldump', '--path' => $path ],
+	qr/error: no start WAL location given/,
+	'path option requires start location');
+command_like(
+	[
+		'pg_waldump',
+		'--path' => $path,
+		'--start' => $start_lsn,
+		'--end' => $end_lsn,
+	],
+	qr/./,
+	'runs with path option and start and end locations');
+command_fails_like(
+	[
+		'pg_waldump',
+		'--path' => $path,
+		'--start' => $start_lsn,
+	],
+	qr/error: error in WAL record at/,
+	'falling off the end of the WAL results in an error');
+
+command_fails_like(
+	[
+		'pg_waldump', '--quiet',
+		'--path' => $path,
+		'--start' => $start_lsn
+	],
+	qr/error: error in WAL record at/,
+	'errors are shown with --quiet');
+
+test_pg_waldump_skip_bytes($path, $start_lsn, $end_lsn);
+
+my @lines = test_pg_waldump($path, $start_lsn, $end_lsn);
+is(grep(!/^rmgr: \w/, @lines), 0, 'all output lines are rmgr lines');
+
+@lines = test_pg_waldump($path, $contrecord_lsn, $end_lsn);
+is(grep(!/^rmgr: \w/, @lines), 0, 'all output lines are rmgr lines');
+
+test_pg_waldump_skip_bytes($path, $contrecord_lsn, $end_lsn);
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--limit' => 6);
+is(@lines, 6, 'limit option observed');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--fullpage');
+is(grep(!/^rmgr:.*\bFPW\b/, @lines), 0, 'all output lines are FPW');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--stats');
+like($lines[0], qr/WAL statistics/, "statistics on stdout");
+is(grep(/^rmgr:/, @lines), 0, 'no rmgr lines output');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--stats=record');
+like($lines[0], qr/WAL statistics/, "statistics on stdout");
+is(grep(/^rmgr:/, @lines), 0, 'no rmgr lines output');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--rmgr' => 'Btree');
+is(grep(!/^rmgr: Btree/, @lines), 0, 'only Btree lines');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--fork' => 'init');
+is(grep(!/fork init/, @lines), 0, 'only init fork lines');
+
+@lines = test_pg_waldump($path, $start_lsn, $end_lsn,
+	'--relation' => "$default_ts_oid/$postgres_db_oid/$rel_t1_oid");
+is(grep(!/rel $default_ts_oid\/$postgres_db_oid\/$rel_t1_oid/, @lines),
+	0, 'only lines for selected relation');
+
+@lines = test_pg_waldump(
+	$path, $start_lsn, $end_lsn,
+	'--relation' => "$default_ts_oid/$postgres_db_oid/$rel_i1a_oid",
+	'--block' => 1);
+is(grep(!/\bblk 1\b/, @lines), 0, 'only lines for selected block');
 
 done_testing();
new file mode 100644
index 00000000000..c615713efd4
--- /dev/null
+++ b/src/bin/pg_waldump/t/003_archive.pl
@@ -0,0 +1,320 @@
+
+# Copyright (c) 2021-2026, PostgreSQL Global Development Group
+
+# Test pg_waldump's ability to read WAL from tar archives.
+
+use strict;
+use warnings FATAL => 'all';
+use Cwd;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use List::Util qw(shuffle);
+
+my $tar = $ENV{TAR};
+
+if (!defined $tar)
+{
+	plan skip_all => 'tar command is not available';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf(
+	'postgresql.conf', q{
+autovacuum = off
+checkpoint_timeout = 1h
+
+# for standbydesc
+archive_mode=on
+archive_command=''
+
+# for XLOG_HEAP_TRUNCATE
+wal_level=logical
+});
+$node->start;
+
+my ($start_lsn, $start_walfile) = split /\|/,
+  $node->safe_psql('postgres',
+	q{SELECT pg_current_wal_insert_lsn(), pg_walfile_name(pg_current_wal_insert_lsn())}
+  );
+
+$node->safe_psql(
+	'postgres', q{
+-- heap, btree, hash, sequence
+CREATE TABLE t1 (a int GENERATED ALWAYS AS IDENTITY, b text);
+CREATE INDEX i1a ON t1 USING btree (a);
+CREATE INDEX i1b ON t1 USING hash (b);
+INSERT INTO t1 VALUES (default, 'one'), (default, 'two');
+DELETE FROM t1 WHERE b = 'one';
+TRUNCATE t1;
+
+-- abort
+START TRANSACTION;
+INSERT INTO t1 VALUES (default, 'three');
+ROLLBACK;
+
+-- unlogged/init fork
+CREATE UNLOGGED TABLE t2 (x int);
+CREATE INDEX i2 ON t2 USING btree (x);
+INSERT INTO t2 SELECT generate_series(1, 10);
+
+-- gin
+CREATE TABLE gin_idx_tbl (id bigserial PRIMARY KEY, data jsonb);
+CREATE INDEX gin_idx ON gin_idx_tbl USING gin (data);
+INSERT INTO gin_idx_tbl
+    WITH random_json AS (
+        SELECT json_object_agg(key, trunc(random() * 10)) as json_data
+            FROM unnest(array['a', 'b', 'c']) as u(key))
+          SELECT generate_series(1,500), json_data FROM random_json;
+
+-- gist, spgist
+CREATE TABLE gist_idx_tbl (p point);
+CREATE INDEX gist_idx ON gist_idx_tbl USING gist (p);
+CREATE INDEX spgist_idx ON gist_idx_tbl USING spgist (p);
+INSERT INTO gist_idx_tbl (p) VALUES (point '(1, 1)'), (point '(3, 2)'), (point '(6, 3)');
+
+-- brin
+CREATE TABLE brin_idx_tbl (col1 int, col2 text, col3 text );
+CREATE INDEX brin_idx ON brin_idx_tbl USING brin (col1, col2, col3) WITH (autosummarize=on);
+INSERT INTO brin_idx_tbl SELECT generate_series(1, 10000), 'dummy', 'dummy';
+UPDATE brin_idx_tbl SET col2 = 'updated' WHERE col1 BETWEEN 1 AND 5000;
+SELECT brin_summarize_range('brin_idx', 0);
+SELECT brin_desummarize_range('brin_idx', 0);
+
+VACUUM;
+
+-- logical message
+SELECT pg_logical_emit_message(true, 'foo', 'bar');
+
+-- relmap
+VACUUM FULL pg_authid;
+
+-- database
+CREATE DATABASE d1;
+DROP DATABASE d1;
+});
+
+my $tblspc_path = PostgreSQL::Test::Utils::tempdir_short();
+
+$node->safe_psql(
+	'postgres', qq{
+CREATE TABLESPACE ts1 LOCATION '$tblspc_path';
+DROP TABLESPACE ts1;
+});
+
+# Consume all remaining room in the current WAL segment, leaving space enough
+# only for the start of a largish record, to test contrecord decoding.
+$node->safe_psql(
+	'postgres', q{
+DO $$
+DECLARE
+    wal_segsize int := setting::int FROM pg_settings WHERE name = 'wal_segment_size';
+    remain int;
+    iters  int := 0;
+BEGIN
+    LOOP
+        INSERT into t1(b)
+        select repeat(encode(sha256(g::text::bytea), 'hex'), (random() * 15 + 1)::int)
+        from generate_series(1, 10) g;
+
+        remain := wal_segsize - (pg_current_wal_insert_lsn() - '0/0') % wal_segsize;
+        IF remain < 2 * setting::int from pg_settings where name = 'block_size' THEN
+            RAISE log 'exiting after % iterations, % bytes to end of WAL segment', iters, remain;
+            EXIT;
+        END IF;
+        iters := iters + 1;
+    END LOOP;
+END
+$$;
+});
+
+my $contrecord_lsn =
+  $node->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn()');
+$node->safe_psql('postgres',
+	qq{SELECT pg_logical_emit_message(true, 'test 026', repeat('xyzxz', 123456))}
+);
+
+my ($end_lsn, $end_walfile) = split /\|/,
+  $node->safe_psql('postgres',
+	q{SELECT pg_current_wal_insert_lsn(), pg_walfile_name(pg_current_wal_insert_lsn())}
+  );
+
+$node->stop;
+
+
+sub test_pg_waldump_skip_bytes
+{
+	my ($path, $startlsn, $endlsn) = @_;
+
+	my ($part1, $part2) = split qr{/}, $startlsn;
+	my $lsn2 = hex $part2;
+	$lsn2++;
+	my $new_start = sprintf("%s/%X", $part1, $lsn2);
+
+	my ($stdout, $stderr);
+
+	my $result = IPC::Run::run [
+		'pg_waldump',
+		'--start' => $new_start,
+		'--end' => $endlsn,
+		'--path' => $path,
+	  ],
+	  '>' => \$stdout,
+	  '2>' => \$stderr;
+	ok($result, "runs with start segment and start LSN specified");
+	like($stderr, qr/first record is after/, 'info message printed');
+}
+
+sub test_pg_waldump
+{
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+	my ($path, $startlsn, $endlsn, @opts) = @_;
+
+	my ($stdout, $stderr);
+
+	my $result = IPC::Run::run [
+		'pg_waldump',
+		'--start' => $startlsn,
+		'--end' => $endlsn,
+		'--path' => $path,
+		@opts
+	  ],
+	  '>' => \$stdout,
+	  '2>' => \$stderr;
+	ok($result, "pg_waldump @opts: runs ok");
+	is($stderr, '', "pg_waldump @opts: no stderr");
+	my @lines = split /\n/, $stdout;
+	ok(@lines > 0, "pg_waldump @opts: some lines are output");
+	return @lines;
+}
+
+sub generate_archive
+{
+	my ($archive, $directory, $compression_flags) = @_;
+
+	my @files;
+	opendir my $dh, $directory or die "opendir: $!";
+	while (my $entry = readdir $dh)
+	{
+		next if $entry eq '.' || $entry eq '..';
+		push @files, $entry;
+	}
+	closedir $dh;
+
+	@files = shuffle @files;
+
+	my $cwd = getcwd;
+	chdir($directory) || die "chdir: $!";
+	command_ok([ $tar, $compression_flags, $archive, @files ],
+		"create archive $archive");
+	chdir($cwd) || die "chdir: $!";
+}
+
+
+my $tmp_dir = PostgreSQL::Test::Utils::tempdir_short();
+
+my @scenarios = (
+	{
+		'path' => "$tmp_dir/pg_wal.tar",
+		'compression_method' => 'none',
+		'compression_flags' => '-cf',
+		'enabled' => 1,
+	},
+	{
+		'path' => "$tmp_dir/pg_wal.tar.gz",
+		'compression_method' => 'gzip',
+		'compression_flags' => '-czf',
+		'enabled' => check_pg_config("#define HAVE_LIBZ 1"),
+	});
+
+for my $scenario (@scenarios)
+{
+	my $path = $scenario->{'path'};
+	my $method = $scenario->{'compression_method'};
+
+  SKIP:
+	{
+		skip "$method compression not supported by this build", 1
+		  if !$scenario->{'enabled'};
+
+		generate_archive(
+			$path,
+			$node->data_dir . '/pg_wal',
+			$scenario->{'compression_flags'});
+
+		command_fails_like(
+			[ 'pg_waldump', '--path' => $path ],
+			qr/error: no start WAL location given/,
+			"$method: path option requires start location");
+		command_like(
+			[
+				'pg_waldump',
+				'--path' => $path,
+				'--start' => $start_lsn,
+				'--end' => $end_lsn,
+			],
+			qr/./,
+			"$method: runs with path option and start and end locations");
+		command_fails_like(
+			[
+				'pg_waldump',
+				'--path' => $path,
+				'--start' => $start_lsn,
+			],
+			qr/error: error in WAL record at/,
+			"$method: falling off the end of the WAL results in an error");
+
+		command_fails_like(
+			[
+				'pg_waldump', '--quiet',
+				'--path' => $path,
+				'--start' => $start_lsn
+			],
+			qr/error: error in WAL record at/,
+			"$method: errors are shown with --quiet");
+
+		test_pg_waldump_skip_bytes($path, $start_lsn, $end_lsn);
+
+		my @lines = test_pg_waldump($path, $start_lsn, $end_lsn);
+		is(grep(!/^rmgr: \w/, @lines),
+			0, "$method: all output lines are rmgr lines");
+
+		@lines = test_pg_waldump($path, $contrecord_lsn, $end_lsn);
+		is(grep(!/^rmgr: \w/, @lines),
+			0, "$method: contrecord - all output lines are rmgr lines");
+
+		test_pg_waldump_skip_bytes($path, $contrecord_lsn, $end_lsn);
+
+		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--limit' => 6);
+		is(@lines, 6, "$method: limit option observed");
+
+		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--fullpage');
+		is(grep(!/^rmgr:.*\bFPW\b/, @lines),
+			0, "$method: all output lines are FPW");
+
+		@lines = test_pg_waldump($path, $start_lsn, $end_lsn, '--stats');
+		like($lines[0], qr/WAL statistics/, "$method: statistics on stdout");
+		is(grep(/^rmgr:/, @lines), 0, "$method: no rmgr lines output");
+
+		@lines =
+		  test_pg_waldump($path, $start_lsn, $end_lsn, '--stats=record');
+		like($lines[0], qr/WAL statistics/,
+			"$method: stats=record on stdout");
+		is(grep(/^rmgr:/, @lines),
+			0, "$method: no rmgr lines with stats=record");
+
+		@lines =
+		  test_pg_waldump($path, $start_lsn, $end_lsn, '--rmgr' => 'Btree');
+		is(grep(!/^rmgr: Btree/, @lines), 0, "$method: only Btree lines");
+
+		@lines =
+		  test_pg_waldump($path, $start_lsn, $end_lsn, '--fork' => 'init');
+		is(grep(!/fork init/, @lines), 0, "$method: only init fork lines");
+
+		# Cleanup.
+		unlink $path;
+	}
+}
+
+done_testing();
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Tue, 3 Mar 2026 00:00:00 +0000
Subject: [PATCH] Fix documentation for pg_waldump tar archive support

Two documentation issues with the tar archive reading feature:

- pg_waldump.sgml: When reading WAL from a tar archive with
  out-of-order segments, pg_waldump spills to temporary files.  TMPDIR
  controls where those files are created, but this was not documented
  in the Environment section.

- pg_verifybackup.sgml: The --wal-path option description still only
  said "directory" even though it now also accepts tar archives.
---
 doc/src/sgml/ref/pg_verifybackup.sgml |  7 ++++---
 doc/src/sgml/ref/pg_waldump.sgml      | 11 +++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/ref/pg_verifybackup.sgml b/doc/src/sgml/ref/pg_verifybackup.sgml
index 16b50b5a4df..1695cfe91c8 100644
--- a/doc/src/sgml/ref/pg_verifybackup.sgml
+++ b/doc/src/sgml/ref/pg_verifybackup.sgml
@@ -261,9 +261,10 @@ PostgreSQL documentation
       <term><option>--wal-path=<replaceable class="parameter">path</replaceable></option></term>
       <listitem>
        <para>
-        Try to parse WAL files stored in the specified directory, rather than
-        in <literal>pg_wal</literal>. This may be useful if the backup is
-        stored in a separate location from the WAL archive.
+        Try to parse WAL files stored in the specified directory or tar
+        archive, rather than in <literal>pg_wal</literal>. This may be
+        useful if the backup is stored in a separate location from the WAL
+        archive.
        </para>
       </listitem>
      </varlistentry>
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index b36323dde92..9bbb4bd5772 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -391,6 +391,17 @@ PostgreSQL documentation
      </para>
     </listitem>
    </varlistentry>
+
+   <varlistentry>
+    <term><envar>TMPDIR</envar></term>
+    <listitem>
+     <para>
+      Directory in which to create temporary files when reading WAL from a
+      tar archive with out-of-order segment files. If not set, the temporary
+      directory is created within the same directory as the tar archive.
+     </para>
+    </listitem>
+   </varlistentry>
   </variablelist>
  </refsect1>
 
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Tue, 3 Mar 2026 00:00:00 +0000
Subject: [PATCH] Fix bugs in pg_waldump tar archive support

Fix several bugs introduced by the pg_waldump archive WAL reading
feature:

- pg_waldump.c: The error path for verify_directory() printed waldir
  (which is NULL when --path is used) instead of walpath.

- archive_waldump.c: The error message for short reads had an operator
  precedence bug: (long long int) count - nbytes cast only count, not
  the subtraction result.  Also reported nbytes (the requested amount)
  instead of count (the total file size) for the "of" portion.

- archive_waldump.c: The "ignoring duplicate WAL" code path leaked
  fname (allocated via pnstrdup/palloc).  Also changed the existing
  free(fname) to pfree(fname) for consistency.

- pg_verifybackup.c: The rename from --wal-directory to --wal-path
  didn't preserve the old spelling as a backward-compatible alias.

- pg_verifybackup.c: Fix double space before "Or" in --wal-path
  error hint message.
---
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 935ab8fafa8..b0b764913cf 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -131,6 +131,7 @@ main(int argc, char **argv)
 		{"quiet", no_argument, NULL, 'q'},
 		{"skip-checksums", no_argument, NULL, 's'},
 		{"wal-path", required_argument, NULL, 'w'},
+		{"wal-directory", required_argument, NULL, 'w'},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -376,7 +377,7 @@ main(int argc, char **argv)
 		else
 		{
 			pg_log_error("WAL archive not found");
-			pg_log_error_hint("Specify the correct path using the option -w/--wal-path.  "
+			pg_log_error_hint("Specify the correct path using the option -w/--wal-path. "
 							  "Or you must use -n/--no-parse-wal when verifying a tar-format backup.");
 			exit(1);
 		}
diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index c5a4485b5b1..1479efe61f5 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -344,8 +344,8 @@ read_archive_wal_page(XLogDumpPrivate *privateInfo, XLogRecPtr targetPagePtr,
 				read_archive_file(privateInfo, READ_CHUNK_SIZE) == 0)
 				pg_fatal("could not read file \"%s\" from archive \"%s\": read %lld of %lld",
 						 fname, privateInfo->archive_name,
-						 (long long int) count - nbytes,
-						 (long long int) nbytes);
+						 (long long int) (count - nbytes),
+						 (long long int) count);
 		}
 	}
 
@@ -664,7 +664,7 @@ astreamer_waldump_content(astreamer *streamer, astreamer_member *member,
 						privateInfo->start_segno > segno ||
 						privateInfo->end_segno < segno)
 					{
-						free(fname);
+						pfree(fname);
 						break;
 					}
 				}
@@ -680,6 +680,7 @@ astreamer_waldump_content(astreamer *streamer, astreamer_member *member,
 				{
 					pg_log_warning("ignoring duplicate WAL \"%s\" found in archive \"%s\"",
 								   member->pathname, privateInfo->archive_name);
+					pfree(fname);
 					break;
 				}
 
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 114969217d8..4b438b53ead 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -1223,7 +1223,7 @@ main(int argc, char **argv)
 		/* validate path points to directory */
 		else if (!verify_directory(walpath))
 		{
-			pg_log_error("could not open directory \"%s\": %m", waldir);
+			pg_log_error("could not open directory \"%s\": %m", walpath);
 			goto bad_argument;
 		}
 	}

---

From: Amul Sul <[email-masked]>
Date: Wed, 4 Mar 2026 18:22:00 +0530

> I think this looks in pretty good shape.

Thank you very much for looking at the patch.

> case we could just live with things as they are.

I agree that the reported skip number was incorrect, and I have
corrected it in the attached patch. I haven't applied your patch for
the TAP test improvements yet because I wanted to double-check it
first with you; the patch as it stood created duplicate tests already
present in 001_basic.pl. To avoid this duplication, I have added a
loop that performs tests for both plain and tar WAL directory inputs,
similar to the approach used in pg_verifybackup for different
compression type tests (e.g., 008_untar.pl, 010_client_untar.pl). I
don't have any objection to doing so if you feel the duplication is
acceptable, but I feel that using a loop for the tests in 001_basic.pl
is a bit tidier. Let me know your thoughts.

I have applied all your other patches but skipped the changes to
pg_verifybackup.c from cf5955-fixes.patch.no-cfbot, as they seem
unrelated or perhaps I have misunderstood them.

Regards,
Amul

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 4 Mar 2026 16:50:30 -0500

On 2026-03-04 We 7:52 AM, Amul Sul wrote:
> On Wed, Mar 4, 2026 at 6:07 AM Andrew Dunstan<[email-masked]> wrote:
>>
>>> On Wed, Feb 18, 2026 at 12:28 PM Amul Sul<[email-masked]> wrote:
>>>> On Tue, Feb 10, 2026 at 3:06 PM Amul Sul<[email-masked]> wrote:
>>>>> On Wed, Feb 4, 2026 at 6:39 PM Amul Sul<[email-masked]> wrote:
>>>>>> On Wed, Jan 28, 2026 at 2:41 AM Robert Haas<[email-masked]> wrote:
>>>>>>> On Tue, Jan 27, 2026 at 7:07 AM Amul Sul<[email-masked]> wrote:
>>>> 1]http://postgr.es/m/[email-masked]
>>>>

I will take a look.

> unrelated or perhaps I have misunderstood them.

<brown-paper-bag> That's what I get for using a poorly written tool.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

---

From: Amul Sul <[email-masked]>
Date: Mon, 9 Mar 2026 17:56:50 +0530

On Sat, Mar 7, 2026 at 3:51 AM Andrew Dunstan <[email-masked]> wrote:
>
>
> On 2026-03-04 We 4:50 PM, Andrew Dunstan wrote:
> >
> >
> I'm ok, with doing it this way. It's just a bit fragile - if we add a
> test the number will be wrong. But maybe it's not worth worrying about.
>
> Everything else looks fairly good. The attached fixes a few relatively
> minor issues in v15. The main one is that it stops allocating/freeing a
> buffer every time we call read_archive_file() and instead adds a
> reusable buffer. It also adds back wal-directory as an undocumented
> alias of wal-path, to avoid breaking legacy scripts unnecessarily, and
> adds constness to the fname argument of pg_tar_compress_algorithm, as
> well as fixing some indentation and grammar issues.
>
> All in all I think we're in good shape.

Thanks for the review. I have incorporated your suggested changes,
with one exception: I have skipped the buffer reallocation code in
read_archive_file(). Since we only handle two specific read sizes --
XLOG_BLCKSZ and READ_CHUNK_SIZE (128 KB, we defined in
archive_waldump.c) -- dynamic reallocation seems unnecessary. Instead,
I moved the allocation to init_archive_reader(), which now initializes
a buffer at READ_CHUNK_SIZE. I also added an assertion in
read_archive_file() to ensure that no read request exceeds this
allocated capacity.

Kindly have a look at the attached version and let me know your thoughts.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Wed, 18 Mar 2026 17:15:47 +0530

On Wed, Mar 11, 2026 at 10:38 PM Andrew Dunstan <[email-masked]> wrote:
>
>
> On 2026-03-09 Mo 8:26 AM, Amul Sul wrote:
>
> Looks pretty good. I have squashed them into three patches I think are committable. Also attached is a diff showing what's changed - mainly this:
>
> . --follow + tar archive rejected (pg_waldump.c) — new validation prevents a confusing pg_fatal when combining --follow with a tar archive
> . error messages split (archive_waldump.c) — the single "could not read file" error is now two distinct messages: "WAL segment is too short" (truncated file) vs "unexpected end of archive" (archive EOF) - Fixes an issue raised in review
> . hash table cleanup (archive_waldump.c) — free_archive_reader now iterates and frees all remaining hash entries and destroys the table
>

The final squashed version looks good to me, thank you. But, I would
like to propose splitting the 0001 patch into two separate commits: a
preparatory refactoring of the pg_waldump code and a standalone commit
that moves the tar archive detection and compression logic to a common
location, as the latter is an independent improvement to the existing
codebase. Additionally, since the test file refactoring was only kept
separate to facilitate the review and has already been reviewed, I
suggest merging those changes into the main feature patch i.e. 0002.
All other elements should remain in a single preparatory refactoring
patch for pg_waldump.

Attached is the version that includes the proposed split. No
additional changes to 0002 and 0003 patches.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Wed, 18 Mar 2026 20:46:35 +0530

On Wed, Mar 18, 2026 at 5:15 PM Amul Sul <[email-masked]> wrote:
> > [...]

Added the two missing 'Reviewed-by' lines to the credit section of the
commit message and did a minor optimization in get_archive_wal_entry.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Thu, 19 Mar 2026 15:50:59 +0530

> commit message and did a minor optimization in get_archive_wal_entry.

Attaching an updated version. It includes some tweaks to code
comments, adds an assert inside get_archive_wal_entry(), moves the
archive_read_buf_size declaration and usage into an assert-enabled
check, and makes a minor change to precheck_tar_backup_file() to
assign out-variables only after successful validation.

Regards,
Amul

---

From: Zsolt Parragi <[email-masked]>
Date: Thu, 19 Mar 2026 20:48:27 +0000

Hello!

Path is ignored with a positional argument, I think this is a bug?

This fails:

pg_waldump --path /wal/dir 000000010000000000000001

And this works:

pg_waldump --path /wal/dir --start 0/01000028 --end 0/010020F8

+{
+ int fname_len = strlen(fname);
+

Shouldn't this use size_t?

+ /*
+ * Setup temporary directory to store WAL segments and set up an exit
+ * callback to remove it upon completion.
+ */
+ setup_tmpwal_dir(waldir);

Maybe this could be deferred to be created only on first use? If I
understand correctly, in a typical scenario waldump won't use this
temporary directory, yet it always creates it.

---

From: Amul Sul <[email-masked]>
Date: Fri, 20 Mar 2026 17:01:00 +0530

> pg_waldump --path /wal/dir --start 0/01000028 --end 0/010020F8

Good catch! I've fixed this in the attached version and updated a test
case to cover this scenario.

> Shouldn't this use size_t?

Okay, that can be used. I’ve done the same in the attached version.

> temporary directory, yet it always creates it.

Yeah, that optimization can be done, but passing the waldir  -- which
is only used once -- to the point where the first temp file is created
would require quite a bit of code refactoring that doesn't seem to
offer much gain, IMO.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Fri, 20 Mar 2026 18:56:42 +0530

> would require quite a bit of code refactoring that doesn't seem to

Since Andrew also leans toward creating the directory only when
needed, I have reconsidered the approach. I think we can pass waldir
(the archive directory) via XLogDumpPrivate, and I’ve implemented that
in the attached version.

Regards,
Amul

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 20 Mar 2026 15:33:18 -0400

> (the archive directory) via XLogDumpPrivate, and I’ve implemented that

Thanks, committed with very minor tweaks.

cheers

andrew

---

From: Amul Sul <[email-masked]>
Date: Sat, 21 Mar 2026 11:49:28 +0530

On Sat, Mar 21, 2026 at 9:19 AM Tom Lane <[email-masked]> wrote:
>
> Andrew Dunstan <[email-masked]> writes:
> Buildfarm members batta and hachi don't like this very much.
> They fail the pg_verifybackup tests like so:
>
> # Running: pg_verifybackup --exit-on-error /home/admin/batta/buildroot/HEAD/pgsql.build/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/server-backup
> pg_waldump: error: could not find WAL in archive "base.tar.zst"
> Only the zstd-compression case fails.  I've spent several hours trying
> to reproduce this, without any luck, although I can get a similar
> failure in only the gzip case if I build with --with-wal-blocksize=64.
> I do not have an explanation for the seeming cross-platform
> difference.  However after adding a lot of debug tracing, I believe
> I see the bug, or at least a related bug.  This bit in
> archive_waldump.c's init_archive_reader is where the error comes from:
>
>     /*
>      * Read until we have at least one full WAL page (XLOG_BLCKSZ bytes) from
>      * the first WAL segment in the archive so we can extract the WAL segment
>      * size from the long page header.
>      */
>     while (entry == NULL || entry->buf->len < XLOG_BLCKSZ)
>     {
>         if (read_archive_file(privateInfo, XLOG_BLCKSZ) == 0)
>             pg_fatal("could not find WAL in archive \"%s\"",
>         entry = privateInfo->cur_file;
>     }
>
> That looks plausible but is in fact utterly broken when there's not a
> lot of WAL data in the archive, as there is not in this test case.
> There are at least two problems:
>

Thanks for the detailed debugging. I noticed the failure this morning
and had started investigating the issue, but in the meantime, I got
your helpful reply, which saved me a bunch of time and energy.

> 1. read_archive_file reads some data from the source WAL archive and
> shoves it into the astreamer decompression pipeline.  However, once it
> runs out of source data, it just returns zero and we fail immediately.
> This does not account for the possibility --- nay, certainty --- that
> there is data queued inside the decompression pipeline.  So this
> doesn't work if the data we need has been compressed into less than
> XLOG_BLCKSZ worth of compressed data.  (I suppose that the seeming
> cross-platform differences have to do with the effectiveness of the
> compression algorithm, but I don't really understand why it'd not be
> the same everywhere.)  We need to do astreamer_finalize once we run
> out of source data.  I think the cleanest place to handle that would
> be inside read_archive_file, but its return convention will need some
> rework if we want to put it there (because rc == 0 shouldn't cause an
> immediate failure if we were able to finalize some more data).  As an
> ugly experiment I put an astreamer_finalize call into the rc == 0 path
> of the above loop, but it still didn't work, because:
>
> 2. If the decompression pipeline reaches the end of the WAL file that
> we want, the ASTREAMER_MEMBER_TRAILER case in
> astreamer_waldump_content instantly resets privateInfo->cur_file to
> NULL.  Then the loop in init_archive_reader cannot exit successfully,
> and it will just read till the end of the archive and fail.
>
> I see that of the three callers of read_archive_file, only
> get_archive_wal_entry is aware of this possibility; but
> init_archive_reader certainly needs to deal with it and I bet
> read_archive_wal_page does too.  Moreover, get_archive_wal_entry's
> solution looks to me like a fragile kluge that probably doesn't work
> reliably either, the reason being that privateInfo->cur_file can
> change multiple times during a single call to read_archive_file,
> if the WAL data has been compressed sufficiently.  That whole API
> seems to need some rethinking, not to mention better documentation
> than the zero it has now.
>

I agree; init_archive_reader needs that handling, but
read_archive_wal_page doesn't need any fix. Since it only deals with
the current entry and already holds a reference to it, there is no
need to fetch it from the hash table again.

init_archive_reader has to scan the hash table because it doesn't
already have the specific WAL filename it is looking for, unlike
get_archive_wal_entry. Please have a look at the attached patch, which
tries to fix that.

> While I'm bitching: this error message "could not find WAL in archive
> \"%s\"" seems to me to be completely misleading and off-point.
>

I tried to improve that in the attached version.

regards,
Amul

---

From: Michael Paquier <[email-masked]>
Date: Sat, 21 Mar 2026 15:23:41 +0900

> pg_verifybackup: error: WAL parsing failed for timeline 1

I did not look at what's happening on the host, but it seems like a
safe bet to assume that we are not seeing many failures in the
buildfarm because we don't have many animals that have the idea to add
--with-zstd to their build configuration, like these two ones.
--
Michael

---

From: Amul Sul <[email-masked]>
Date: Sat, 21 Mar 2026 21:05:50 +0530

On Sat, Mar 21, 2026 at 5:51 PM Andrew Dunstan <[email-masked]> wrote:
>
>
> On 2026-03-21 Sa 2:34 AM, Tom Lane wrote:
>
> Michael Paquier <[email-masked]> writes:
>
> That may be part of the story, but only part.  I spent a good deal of
> time trying to reproduce batta & hachi's configurations locally, on
> several different platforms, but still couldn't duplicate what they
> are showing.
>
>
>
>
>
> Yeah, I haven't been able to reproduce it either. But while investigating I found a couple of issues. We neglected to add one of the tests to meson.build, and we neglected to close some files, causing errors on windows.
>

While the proposed fix of closing the file pointer before returning is
correct, we also need to ensure the file is reopened in the next call
to spill any remaining buffered data. I’ve made a small update to
Andrew's 0001 patch to handle this. Also, changes to meson.build don't
seem to be needed as we haven't committed that file yet (unless I am
missing something).

I’ve also reattached the other patches so they don't get lost: v2-0002
is Andrew's patch for the archive streamer, and v2-0003 is the patch I
posted previously [1].

Regards,
Amul

1] http://postgr.es/m/[email-masked]

---

From: Amul Sul <[email-masked]>
Date: Sat, 21 Mar 2026 22:56:38 +0530

> is Andrew's patch for the archive streamer, and v2-0003 is the patch I

On further thought, I don't think v2-0001 is the right patch. Consider
the case where we write a temporary file partially: if the next
segment required for decoding is that same segment,
TarWALDumpReadPage() will find the physical file present and continue
decoding, potentially triggering an error later due to the shorter
file.

I have attached the v3-0001 patch, which ensures that once we start
writing a temporary file, it should be finished before performing the
lookup. This ensures we don't leave a partial file on disk.

Updated patches are attached; 0002 and 0003 remain the same as before.

Regards,
Amul

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 07:24:56 -0400

On Sun, Mar 22, 2026 at 12:24 AM Tom Lane <[email-masked]> wrote:

> I wrote:
> > Unsurprisingly, applying this change to unmodified master results
> > in the pg_waldump and pg_verifybackup tests falling over.  More
> > surprisingly, they still fall over after applying your fix to the
> > decompressors, so there's some other source of garbage trailing
> > data.  I haven't figured out what.
>
> In the learn-something-new-every-day dept.: good ol' GNU tar itself
> does that.  By default, it zero-pads its output to a multiple of 10kB
> after it's written the required terminator.  Moreover, this behavior
> is actually specified by POSIX:
>
>   -x format
>     Specify the output archive format. The pax utility shall support
>     the following formats:
>     ...
>     ustar
>       The tar interchange format; see the EXTENDED DESCRIPTION
>       section. The default blocksize for this format for character
>       special archive files shall be 10240. Implementations shall
>       support all blocksize values less than or equal to 32256 that
>       are multiples of 512.
>
> So, astreamer_tar_parser_content's idea that it should disallow more
> than 1024 bytes of trailer is completely wrong, which we would have
> figured out long ago if the code attempting to enforce that weren't
> completely broken.
>
> You could argue that this means the tar files our existing utilities
> create aren't POSIX-compliant.  I think it's all right though: we
> can just say that we write these files with blocksize 1024 not
> blocksize 10240, and tar-file readers are required to accept that
> per the above spec text.
>
> However, this discourages me from editorializing on the file trailer
> emitted by whatever wrote the tar file we are reading.  I think
> emitting it as-is is the most appropriate thing.  So we should just
> get rid of astreamer_tar_parser_content's nonfunctional error check
> and not change its behavior otherwise.
>
>
>
OK, patch 5 of this set does that. I reworked your previous patches 2 and 3
slightly - mostly additional comments, and fixing a bug in use
of sizeof(XLogLongPageHeader). Patch 4 here tries to fix the wrong use of
cur_file in get_archive_wal_entry()

cheers

andrew
From 51d53b166df7c8eaebe49756e24088c16764807b Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 06:53:25 -0400
Subject: [PATCH v5 3/5] Fix init_archive_reader to not depend on cur_file.

init_archive_reader() relied on privateInfo->cur_file to track which
WAL segment was being read, but cur_file can become NULL if a member
trailer is processed during a read_archive_file() call.  This could
cause unreproducible "could not find WAL in archive" failures,
particularly with compressed archives where all the WAL data fits
in a small number of compressed bytes.

Fix by scanning the hash table after each read to find any cached
WAL segment with sufficient data, instead of depending on cur_file.
Also reduce the minimum data requirement from XLOG_BLCKSZ to
sizeof(XLogLongPageHeaderData), since we only need the long page
header to extract the segment size.

Add a safety comment on cur_file in pg_waldump.h to document that
it can change during a single read_archive_file() call.

Author: Tom Lane <[email-masked]>

Discussion: https://postgr.es/m/[email-masked]
---
 src/bin/pg_waldump/archive_waldump.c | 22 +++++++++++++++++-----
 src/bin/pg_waldump/pg_waldump.h      |  9 ++++++++-
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index cd092a057ef..3fce2183099 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -173,17 +173,29 @@ init_archive_reader(XLogDumpPrivate *privateInfo,
 	privateInfo->archive_wal_htab = ArchivedWAL_create(8, NULL);
 
 	/*
-	 * Read until we have at least one full WAL page (XLOG_BLCKSZ bytes) from
-	 * the first WAL segment in the archive so we can extract the WAL segment
-	 * size from the long page header.
+	 * Read until we have at least one WAL segment with enough data to extract
+	 * the WAL segment size from the long page header.
+	 *
+	 * We must not rely on cur_file here, because it can become NULL if a
+	 * member trailer is processed during a read_archive_file() call. Instead,
+	 * scan the hash table after each read to find any entry with sufficient
+	 * data.
 	 */
-	while (entry == NULL || entry->buf->len < XLOG_BLCKSZ)
+	while (entry == NULL)
 	{
+		ArchivedWAL_iterator iter;
+
 		if (!read_archive_file(privateInfo, XLOG_BLCKSZ))
 			pg_fatal("could not find WAL in archive \"%s\"",
 					 privateInfo->archive_name);
 
-		entry = privateInfo->cur_file;
+		ArchivedWAL_start_iterate(privateInfo->archive_wal_htab, &iter);
+		while ((entry = ArchivedWAL_iterate(privateInfo->archive_wal_htab,
+											&iter)) != NULL)
+		{
+			if (entry->read_len >= sizeof(XLogLongPageHeaderData))
+				break;
+		}
 	}
 
 	/* Extract the WAL segment size from the long page header */
diff --git a/src/bin/pg_waldump/pg_waldump.h b/src/bin/pg_waldump/pg_waldump.h
index cde7c6ca3f2..ca0dfd97168 100644
--- a/src/bin/pg_waldump/pg_waldump.h
+++ b/src/bin/pg_waldump/pg_waldump.h
@@ -44,7 +44,14 @@ typedef struct XLogDumpPrivate
 	Size		archive_read_buf_size;
 #endif
 
-	/* What the archive streamer is currently reading */
+	/*
+	 * The buffer for the WAL file the archive streamer is currently reading,
+	 * or NULL if none.  It is quite risky to examine this anywhere except in
+	 * astreamer_waldump_content(), since it can change multiple times during
+	 * a single read_archive_file() call.  However, it is safe to assume that
+	 * if cur_file is different from a particular ArchivedWALFile of interest,
+	 * then the archive streamer has finished reading that file.
+	 */
 	struct ArchivedWALFile *cur_file;
 
 	/*
-- 
2.43.0

From 9978359771c14b411704d808abc0f602119f0a9f Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 05:31:14 -0400
Subject: [PATCH v5 1/5] Fix finalization of decompressor astreamers.

Send the correct amount of data to the next astreamer, not the
whole allocated buffer size.  It's unclear how we missed this bug;
perhaps the use-cases so far are insensitive to trailing garbage.

Author: Andrew Dunstan <[email-masked]>

Discussion: https://postgr.es/m/[email-masked]
---
 src/fe_utils/astreamer_gzip.c | 9 +++++----
 src/fe_utils/astreamer_lz4.c  | 9 +++++----
 src/fe_utils/astreamer_zstd.c | 2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/fe_utils/astreamer_gzip.c b/src/fe_utils/astreamer_gzip.c
index 2e080c37a58..df392f67cab 100644
--- a/src/fe_utils/astreamer_gzip.c
+++ b/src/fe_utils/astreamer_gzip.c
@@ -347,10 +347,11 @@ astreamer_gzip_decompressor_finalize(astreamer *streamer)
 	 * End of the stream, if there is some pending data in output buffers then
 	 * we must forward it to next streamer.
 	 */
-	astreamer_content(mystreamer->base.bbs_next, NULL,
-					  mystreamer->base.bbs_buffer.data,
-					  mystreamer->base.bbs_buffer.maxlen,
-					  ASTREAMER_UNKNOWN);
+	if (mystreamer->bytes_written > 0)
+		astreamer_content(mystreamer->base.bbs_next, NULL,
+						  mystreamer->base.bbs_buffer.data,
+						  mystreamer->bytes_written,
+						  ASTREAMER_UNKNOWN);
 
 	astreamer_finalize(mystreamer->base.bbs_next);
 }
diff --git a/src/fe_utils/astreamer_lz4.c b/src/fe_utils/astreamer_lz4.c
index 2bc32b42879..605c188007b 100644
--- a/src/fe_utils/astreamer_lz4.c
+++ b/src/fe_utils/astreamer_lz4.c
@@ -397,10 +397,11 @@ astreamer_lz4_decompressor_finalize(astreamer *streamer)
 	 * End of the stream, if there is some pending data in output buffers then
 	 * we must forward it to next streamer.
 	 */
-	astreamer_content(mystreamer->base.bbs_next, NULL,
-					  mystreamer->base.bbs_buffer.data,
-					  mystreamer->base.bbs_buffer.maxlen,
-					  ASTREAMER_UNKNOWN);
+	if (mystreamer->bytes_written > 0)
+		astreamer_content(mystreamer->base.bbs_next, NULL,
+						  mystreamer->base.bbs_buffer.data,
+						  mystreamer->bytes_written,
+						  ASTREAMER_UNKNOWN);
 
 	astreamer_finalize(mystreamer->base.bbs_next);
 }
diff --git a/src/fe_utils/astreamer_zstd.c b/src/fe_utils/astreamer_zstd.c
index f26abcfd0fa..4b43ab795e3 100644
--- a/src/fe_utils/astreamer_zstd.c
+++ b/src/fe_utils/astreamer_zstd.c
@@ -347,7 +347,7 @@ astreamer_zstd_decompressor_finalize(astreamer *streamer)
 	if (mystreamer->zstd_outBuf.pos > 0)
 		astreamer_content(mystreamer->base.bbs_next, NULL,
 						  mystreamer->base.bbs_buffer.data,
-						  mystreamer->base.bbs_buffer.maxlen,
+						  mystreamer->zstd_outBuf.pos,
 						  ASTREAMER_UNKNOWN);
 
 	astreamer_finalize(mystreamer->base.bbs_next);
-- 
2.43.0

From 91ed0e69a7df00be147548973a8172322998b528 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 06:53:34 -0400
Subject: [PATCH v5 4/5] Fix get_archive_wal_entry to handle cur_file
 transitions reliably.

As noted by Tom Lane, get_archive_wal_entry() uses cur_file in an
unsafe way: a single read_archive_file() call can trigger multiple
astreamer callbacks when compression is effective, causing cur_file
to change several times (entry -> NULL -> new entry) within one
call.  The old code captured cur_file before the read and checked
for changes after, but this missed intermediate transitions.  This
could cause spill-file handles to leak or data to not be flushed
when the streamer finishes one segment and starts another within
the same read.

Restructure the spill logic to explicitly track the entry being
spilled (spill_entry) separately from cur_file, and detect
transitions at the top of each loop iteration.  Also ensure spill
file handles are closed on both success and error paths.

Discussion: https://postgr.es/m/[email-masked]
---
 src/bin/pg_waldump/archive_waldump.c | 117 ++++++++++++++++-----------
 1 file changed, 69 insertions(+), 48 deletions(-)

diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index 3fce2183099..93ed856c674 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -463,11 +463,18 @@ free_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
  * found.  If the archive streamer is reading a WAL file from the archive that
  * is not currently needed, that data is spilled to a temporary file for later
  * retrieval.
+ *
+ * Because a single read_archive_file() call may trigger multiple astreamer
+ * callbacks (especially when data compresses well), cur_file can change
+ * several times within one call: from one entry to NULL (member trailer),
+ * and then to a new entry (next member header).  The spill logic below
+ * handles this by flushing and closing per-entry state whenever we detect
+ * that the streamer has moved on.
  */
 static ArchivedWALFile *
 get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
 {
-	ArchivedWALFile *entry = NULL;
+	ArchivedWALFile *spill_entry = NULL;
 	FILE	   *write_fp = NULL;
 
 	/*
@@ -477,6 +484,8 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
 	 */
 	while (1)
 	{
+		ArchivedWALFile *entry;
+
 		/*
 		 * Search hash table.
 		 *
@@ -488,64 +497,76 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
 		entry = ArchivedWAL_lookup(privateInfo->archive_wal_htab, fname);
 
 		if (entry != NULL)
+		{
+			/* Close any open spill file before returning. */
+			if (write_fp != NULL)
+				fclose(write_fp);
 			return entry;
-
-		/*
-		 * Capture the current entry before calling read_archive_file(),
-		 * because cur_file may advance to a new segment during streaming. We
-		 * hold this reference so we can flush any remaining buffer data and
-		 * close the write handle once we detect that cur_file has moved on.
-		 */
-		entry = privateInfo->cur_file;
-
-		/*
-		 * Fetch more data either when no current file is being tracked or
-		 * when its buffer has been fully flushed to the temporary file.
-		 */
-		if (entry == NULL || entry->buf->len == 0)
-		{
-			if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
-				break;			/* archive file ended */
 		}
 
 		/*
-		 * Archive streamer is reading a non-WAL file or an irrelevant WAL
-		 * file.
-		 */
-		if (entry == NULL)
-			continue;
-
-		/*
-		 * The streamer is producing a WAL segment that isn't the one asked
-		 * for; it must be arriving out of order.  Spill its data to disk so
-		 * it can be read back when needed.
-		 */
-		Assert(strcmp(fname, entry->fname) != 0);
-
-		/* Create a temporary file if one does not already exist */
-		if (!entry->spilled)
-		{
-			write_fp = prepare_tmp_write(entry->fname, privateInfo);
-			entry->spilled = true;
-		}
-
-		/* Flush data from the buffer to the file */
-		perform_tmp_write(entry->fname, entry->buf, write_fp);
-		resetStringInfo(entry->buf);
-
-		/*
-		 * If cur_file changed since we captured entry above, the archive
-		 * streamer has finished this segment and moved on.  Close its spill
-		 * file handle so data is flushed to disk before the next segment
-		 * starts writing to a different handle.
+		 * If the streamer has moved on to a different entry than the one we
+		 * were spilling, flush any remaining data for the old entry and close
+		 * its spill file.
 		 */
-		if (entry != privateInfo->cur_file && write_fp != NULL)
+		if (spill_entry != NULL && spill_entry != privateInfo->cur_file)
 		{
+			if (spill_entry->buf->len > 0)
+			{
+				perform_tmp_write(spill_entry->fname, spill_entry->buf,
+								  write_fp);
+				resetStringInfo(spill_entry->buf);
+			}
 			fclose(write_fp);
 			write_fp = NULL;
+			spill_entry = NULL;
 		}
+
+		/*
+		 * If no WAL file is currently being streamed (cur_file is NULL), or
+		 * the current spill entry's buffer has been fully flushed, we need
+		 * more data from the archive.
+		 */
+		if (privateInfo->cur_file == NULL ||
+			(spill_entry != NULL && spill_entry->buf->len == 0))
+		{
+			if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
+				break;			/* archive fully exhausted */
+			continue;			/* re-check hash table and cur_file */
+		}
+
+		/*
+		 * cur_file points to a WAL segment that isn't the one asked for; it
+		 * must be arriving out of order.  Spill its data to disk so it can be
+		 * read back when needed.
+		 */
+		spill_entry = privateInfo->cur_file;
+		Assert(strcmp(fname, spill_entry->fname) != 0);
+
+		/* Create a temporary file if one does not already exist */
+		if (!spill_entry->spilled)
+		{
+			write_fp = prepare_tmp_write(spill_entry->fname, privateInfo);
+			spill_entry->spilled = true;
+		}
+
+		/* Flush data from the buffer to the file */
+		perform_tmp_write(spill_entry->fname, spill_entry->buf, write_fp);
+		resetStringInfo(spill_entry->buf);
+
+		/*
+		 * Read more data from the archive.  This may add data to the current
+		 * spill_entry's buffer, advance cur_file to a new entry, or set
+		 * cur_file to NULL (member trailer).
+		 */
+		if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
+			break;				/* archive fully exhausted */
 	}
 
+	/* Close any open spill file before erroring out. */
+	if (write_fp != NULL)
+		fclose(write_fp);
+
 	/* Requested WAL segment not found */
 	pg_fatal("could not find WAL \"%s\" in archive \"%s\"",
 			 fname, privateInfo->archive_name);
-- 
2.43.0

From 0de5fd971602ae00fd3bd62cf5da0d8f0a3cce5a Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 05:32:45 -0400
Subject: [PATCH v5 2/5] Fix failure to finalize the decompression pipeline at
 archive EOF.

archive_waldump.c called astreamer_finalize() nowhere.  This meant
that any data retained in decompression buffers at the moment we
detect archive EOF would never reach astreamer_waldump_content(),
resulting in surprising failures if we actually need the last few
bytes of the archive file.

To fix, make read_archive_file() do the finalize once it detects
EOF.  Change its API to return a boolean "yes there's more data"
rather than the entirely-misleading raw count of bytes read.

Also document the contract that cur_file can change (or become NULL)
during a single read_archive_file() call, since the decompression
pipeline may produce enough output to trigger multiple astreamer
callbacks.

Author: Tom Lane <[email-masked]>

Discussion: https://postgr.es/m/[email-masked]
---
 src/bin/pg_waldump/archive_waldump.c | 50 +++++++++++++++++++++++-----
 src/bin/pg_waldump/pg_waldump.h      |  1 +
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index b078c2d6960..cd092a057ef 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -89,7 +89,7 @@ typedef struct astreamer_waldump
 
 static ArchivedWALFile *get_archive_wal_entry(const char *fname,
 											  XLogDumpPrivate *privateInfo);
-static int	read_archive_file(XLogDumpPrivate *privateInfo, Size count);
+static bool read_archive_file(XLogDumpPrivate *privateInfo, Size count);
 static void setup_tmpwal_dir(const char *waldir);
 static void cleanup_tmpwal_dir_atexit(void);
 
@@ -139,6 +139,7 @@ init_archive_reader(XLogDumpPrivate *privateInfo,
 		pg_fatal("could not open file \"%s\"", privateInfo->archive_name);
 
 	privateInfo->archive_fd = fd;
+	privateInfo->archive_fd_eof = false;
 
 	streamer = astreamer_waldump_new(privateInfo);
 
@@ -178,7 +179,7 @@ init_archive_reader(XLogDumpPrivate *privateInfo,
 	 */
 	while (entry == NULL || entry->buf->len < XLOG_BLCKSZ)
 	{
-		if (read_archive_file(privateInfo, XLOG_BLCKSZ) == 0)
+		if (!read_archive_file(privateInfo, XLOG_BLCKSZ))
 			pg_fatal("could not find WAL in archive \"%s\"",
 					 privateInfo->archive_name);
 
@@ -236,9 +237,10 @@ free_archive_reader(XLogDumpPrivate *privateInfo)
 	/*
 	 * NB: Normally, astreamer_finalize() is called before astreamer_free() to
 	 * flush any remaining buffered data or to ensure the end of the tar
-	 * archive is reached.  However, when decoding WAL, once we hit the end
-	 * LSN, any remaining buffered data or unread portion of the archive can
-	 * be safely ignored.
+	 * archive is reached.  read_archive_file() may have done so.  However,
+	 * when decoding WAL we can stop once we hit the end LSN, so we may never
+	 * have read all of the input file.  In that case any remaining buffered
+	 * data or unread portion of the archive can be safely ignored.
 	 */
 	astreamer_free(privateInfo->archive_streamer);
 
@@ -384,7 +386,7 @@ read_archive_wal_page(XLogDumpPrivate *privateInfo, XLogRecPtr targetPagePtr,
 						 fname, privateInfo->archive_name,
 						 (long long int) (count - nbytes),
 						 (long long int) count);
-			if (read_archive_file(privateInfo, READ_CHUNK_SIZE) == 0)
+			if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
 				pg_fatal("unexpected end of archive \"%s\" while reading \"%s\": read %lld of %lld bytes",
 						 privateInfo->archive_name, fname,
 						 (long long int) (count - nbytes),
@@ -490,7 +492,7 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
 		 */
 		if (entry == NULL || entry->buf->len == 0)
 		{
-			if (read_archive_file(privateInfo, READ_CHUNK_SIZE) == 0)
+			if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
 				break;			/* archive file ended */
 		}
 
@@ -540,8 +542,22 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
 /*
  * Reads a chunk from the archive file and passes it through the streamer
  * pipeline for decompression (if needed) and tar member extraction.
+ *
+ * count is the maximum amount to try to read this time.  Note that it's
+ * measured in raw file bytes, and may have little to do with how much
+ * comes out of decompression/extraction.
+ *
+ * Returns true if successful, false if there is no more data.
+ *
+ * Callers must be aware that a single call may trigger multiple callbacks
+ * in astreamer_waldump_content, so privateInfo->cur_file can change value
+ * (or become NULL) during a call.  In particular, cur_file is set to NULL
+ * when the ASTREAMER_MEMBER_TRAILER callback fires at the end of a tar
+ * member; it is then set to a new entry when the next WAL member's
+ * ASTREAMER_MEMBER_HEADER callback fires, which may or may not happen
+ * within the same call.
  */
-static int
+static bool
 read_archive_file(XLogDumpPrivate *privateInfo, Size count)
 {
 	int			rc;
@@ -549,6 +565,11 @@ read_archive_file(XLogDumpPrivate *privateInfo, Size count)
 	/* The read request must not exceed the allocated buffer size. */
 	Assert(privateInfo->archive_read_buf_size >= count);
 
+	/* Fail if we already reached EOF in a prior call. */
+	if (privateInfo->archive_fd_eof)
+		return false;
+
+	/* Try to read some more data. */
 	rc = read(privateInfo->archive_fd, privateInfo->archive_read_buf, count);
 	if (rc < 0)
 		pg_fatal("could not read file \"%s\": %m",
@@ -562,8 +583,19 @@ read_archive_file(XLogDumpPrivate *privateInfo, Size count)
 		astreamer_content(privateInfo->archive_streamer, NULL,
 						  privateInfo->archive_read_buf, rc,
 						  ASTREAMER_UNKNOWN);
+	else
+	{
+		/*
+		 * We reached EOF, but there is probably still data queued in the
+		 * astreamer pipeline's buffers.  Flush it out to ensure that we
+		 * process everything.
+		 */
+		astreamer_finalize(privateInfo->archive_streamer);
+		/* Set flag to ensure we don't finalize more than once. */
+		privateInfo->archive_fd_eof = true;
+	}
 
-	return rc;
+	return true;
 }
 
 /*
diff --git a/src/bin/pg_waldump/pg_waldump.h b/src/bin/pg_waldump/pg_waldump.h
index 36893624f53..cde7c6ca3f2 100644
--- a/src/bin/pg_waldump/pg_waldump.h
+++ b/src/bin/pg_waldump/pg_waldump.h
@@ -35,6 +35,7 @@ typedef struct XLogDumpPrivate
 	char	   *archive_dir;
 	char	   *archive_name;	/* Tar archive filename */
 	int			archive_fd;		/* File descriptor for the open tar file */
+	bool		archive_fd_eof; /* Have we reached EOF on archive_fd? */
 
 	astreamer  *archive_streamer;
 	char	   *archive_read_buf;	/* Reusable read buffer for archive I/O */
-- 
2.43.0

From cab3e116c1064afcc16e638555c74f7f460be55b Mon Sep 17 00:00:00 2001
From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 06:53:58 -0400
Subject: [PATCH v5 5/5] Remove nonfunctional tar file trailer size check.

The ASTREAMER_ARCHIVE_TRAILER case in astreamer_tar_parser_content()
intended to reject tar files whose trailer exceeded 2 blocks.  However,
the check compared 'len' after astreamer_buffer_bytes() had already
consumed all the data and set len to 0, so the pg_fatal() could never
fire.

Moreover, per the POSIX specification for the ustar format, the last
physical block of a tar archive is always full-sized, and "logical
records after the two zero logical records may contain undefined data."
GNU tar, for example, zero-pads its output to a 10kB boundary by
default.  So rejecting extra data after the two zero blocks would be
wrong even if the check worked.

Remove the dead check and update the comment to explain why trailing
data is expected and harmless.

Per report from Tom Lane.

Discussion: https://postgr.es/m/[email-masked]
---
 src/fe_utils/astreamer_tar.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/fe_utils/astreamer_tar.c b/src/fe_utils/astreamer_tar.c
index 4b187f0a8c4..f8be5e4ff8a 100644
--- a/src/fe_utils/astreamer_tar.c
+++ b/src/fe_utils/astreamer_tar.c
@@ -236,12 +236,16 @@ astreamer_tar_parser_content(astreamer *streamer, astreamer_member *member,
 
 				/*
 				 * We've seen an end-of-archive indicator, so anything more is
-				 * buffered and sent as part of the archive trailer. But we
-				 * don't expect more than 2 blocks.
+				 * buffered and sent as part of the archive trailer.
+				 *
+				 * Per POSIX, the last physical block of a tar archive is
+				 * always full-sized, so there may be undefined data after the
+				 * two zero blocks that mark end-of-archive.  GNU tar, for
+				 * example, zero-pads to a 10kB boundary by default.  We just
+				 * buffer whatever we receive and pass it along at finalize
+				 * time.
 				 */
 				astreamer_buffer_bytes(streamer, &data, &len, len);
-				if (len > 2 * TAR_BLOCK_SIZE)
-					pg_fatal("tar file trailer exceeds 2 blocks");
 				return;
 
 			default:
-- 
2.43.0

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 22 Mar 2026 17:19:09 -0400

On Sun, Mar 22, 2026 at 2:17 PM Tom Lane <[email-masked]> wrote:

> I wrote:
> > ... We can make this function far simpler
> > and more obviously correct if we just accept that we'll read a
> > WAL file completely before spilling it.  See my proposed
> > alternative to 0004, attached.
>
> Actually, we can make that better yet by not expecting
> get_archive_wal_entry to clean up after init_archive's
> failure to free all irrelevant hashtable entries.
> Better version attached.
>
>
>

Yeah, this looks good.  I know we also still need to do something about
rmtree trying to remove files we haven't closed. But what we have so far in
this set LGTM. If you want to push this I'm good, otherwise I'll look at it
tomorrow or Tuesday.

cheers

andrew

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Mar 2026 12:11:44 +0900

On Sun, Mar 22, 2026 at 11:02:20PM -0400, Tom Lane wrote:
> Proposed patch attached.  There might be an argument for using some
> other size than 256K for the other two decompressors, but my
> inclination is to try to make all three use roughly the same block
> size.  (See also 66ec01dc4.)

The buildfarm has switched mostly to green, except on this one:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoatzin&dt=2026-03-23%2006%3A00%3A42
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Wed, 25 Mar 2026 13:28:26 -0400

Hi,

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoatzin&dt=2026-03-23%2006%3A00%3A42

I think there's a few more failues. Fairywren regularly fails, including in a
run from today.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2026-03-25%2003%3A48%3A06

There also are a lot of CI failures. E.g.

https://cirrus-ci.com/task/6153854431002624
https://api.cirrus-ci.com/v1/artifact/task/6153854431002624/testrun/build/testrun/pg_waldump/001_basic/log/regress_log_001_basic
# Running: pg_waldump --path C:\msys64\tmp\tNfU5IfQ4a/pg_wal.tar.gz --start 0/01806F48 --end 0/03093BD8
[22:46:25.358](3.991s) not ok 160 - runs with path option and start and end locations: exit code 0
[22:46:25.363](0.005s) #   Failed test 'runs with path option and start and end locations: exit code 0'
#   at C:/cirrus/src/bin/pg_waldump/t/001_basic.pl line 399.
[22:46:25.364](0.001s) ok 161 - runs with path option and start and end locations: no stderr
[22:46:25.365](0.001s) not ok 162 - runs with path option and start and end locations: matches
[22:46:25.365](0.000s) #   Failed test 'runs with path option and start and end locations: matches'
#   at C:/cirrus/src/bin/pg_waldump/t/001_basic.pl line 399.
[22:46:25.366](0.000s) #                   ''
#     doesn't match '(?^:.)'

I was first suspecting that this is due to

commit 1c162c965a1
Author: Fujii Masao <[email-masked]>
Date:   2026-03-24 22:33:09 +0900

    Report detailed errors from XLogFindNextRecord() failures.

but there are afaict failures from before that:
https://cirrus-ci.com/task/5501609960013824
which is for 4019f725f5d, preceding 1c162c965a1
and
https://cirrus-ci.com/task/5317196043255808

It does feel however the failure frequency has increased substantially:
https://cirrus-ci.com/github/postgres/postgres/master

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Sun, 29 Mar 2026 10:36:06 +1300

> I think there's a few more failues. Fairywren regularly fails, including in a

This fails 100% of the time on my machine, even after e9d72348 and ff84efe4, eg:

# Running: pg_waldump --path /tmp/D8WG1Sv2HE/pg_wal.tar --start
0/017A2610 --end 0/02093848
[09:43:29.288](0.148s) not ok 104 - runs with path option and start
and end locations: exit code 0
[09:43:29.289](0.001s) #   Failed test 'runs with path option and
start and end locations: exit code 0'
#   at /home/tmunro/projects/postgresql/src/bin/pg_waldump/t/001_basic.pl
line 402.
[09:43:29.290](0.001s) not ok 105 - runs with path option and start
and end locations: no stderr
[09:43:29.291](0.001s) #   Failed test 'runs with path option and
start and end locations: no stderr'
#   at /home/tmunro/projects/postgresql/src/bin/pg_waldump/t/001_basic.pl
line 402.
[09:43:29.291](0.000s) #          got: 'pg_waldump: error: could not
find WAL "000000010000000000000002" in archive "pg_wal.tar"
# '

I can see that it is wrong about the contents of the tar file:

$ pg_waldump --path _tmp_H_1gv81G1L_pg_wal.tar --start 0/017A2610
--end 0/020934F8 2>&1 | tail -3
rmgr: Hash        len (rec/tot):     72/    72, tx:        720, lsn:
0/01FFC1B8, prev 0/01FFC178, desc: INSERT off 40, blkref #0: rel
1663/5/16397 blk 2, blkref #1: rel
1663/5/16397 blk 0
rmgr: Transaction len (rec/tot):     46/    46, tx:        720, lsn:
0/01FFC200, prev 0/01FFC1B8, desc: COMMIT 2026-03-29 10:15:24.112967
NZDT
pg_waldump: error: could not find WAL "000000010000000000000002" in
archive "_tmp_H_1gv81G1L_pg_wal.tar"

$ tar tvf _tmp_H_1gv81G1L_pg_wal.tar
drwx------  0 tmunro tmunro      0 Mar 29 10:15 archive_status/
-rw-------  0 tmunro tmunro      0 Mar 29 10:15
archive_status/000000010000000000000002.ready
-rw-------  0 tmunro tmunro      0 Mar 29 10:15
archive_status/000000010000000000000001.ready
drwx------  0 tmunro tmunro      0 Mar 29 10:08 summaries/
-rw-------  0 tmunro tmunro 16777216 Mar 29 10:15 000000010000000000000002
-rw-------  0 tmunro tmunro 16777216 Mar 29 10:15 000000010000000000000001
-rw-------  0 tmunro tmunro 16777216 Mar 29 10:15 000000010000000000000003

It seems like the place we'd be looking for the file is in
astreamer_tar_header(), so I added in some caveman debugging:

    /*
     * Parse key fields out of the header.
     */
fprintf(stderr, "XXXX [%s] XXXX\n", &buffer[TAR_OFFSET_NAME]);
    strlcpy(member->pathname, &buffer[TAR_OFFSET_NAME], MAXPGPATH);
    if (member->pathname[0] == '\0')
        pg_fatal("tar member has empty name");

Now I see:

XXXX [archive_status/] XXXX
XXXX [archive_status/000000010000000000000002.ready] XXXX
XXXX [archive_status/000000010000000000000001.ready] XXXX
XXXX [summaries/] XXXX
XXXX [PaxHeader/000000010000000000000002] XXXX
XXXX [GNUSparseFile.0/000000010000000000000002] XXXX
XXXX [000000010000000000000001] XXXX
rmgr: XLOG        len (rec/tot):     30/    30, tx:          0, lsn:
0/017A2610, prev 0/017A25F0, desc: NEXTOID 24576
rmgr: Standby     len (rec/tot):     42/    42, tx:        692, lsn:
0/017A2630, prev 0/017A2610, desc: LOCK xid 692 db 5 rel 16384
rmgr: Storage     len (rec/tot):     42/    42, tx:        692, lsn:
0/017A2660, prev 0/017A2630, desc: CREATE base/5/16384
... lots more normal output ...
rmgr: Hash        len (rec/tot):     72/    72, tx:        720, lsn:
0/01FFBED8, prev 0/01FFBE98, desc: INSERT off 97, blkref #0: rel
1663/5/16397 blk 2, blkref #1: rel
1663/5/16397 blk 0
rmgr: Heap        len (rec/tot):    575/   575, tx:        720, lsn:
0/01FFBF20, prev 0/01FFBED8, desc: INSERT off: 12, flags: 0x08, blkref
#0: rel 1663/5/16393 blk 52
rmgr: Btree       len (rec/tot):     64/    64, tx:        720,
lsn:XXXX [PaxHeader/000000010000000000000003] XXXX
XXXX [GNUSparseFile.0/000000010000000000000003] XXXX
 0/01FFC178, prev 0/01FFBF20, desc: INSERT_LEAF off: 344, blkref #0:
rel 1663/5/16396 blk 2
rmgr: Hash        len (rec/tot):     72/    72, tx:        720, lsn:
0/01FFC1B8, prev 0/01FFC178, desc: INSERT off 40, blkref #0: rel
1663/5/16397 blk 2, blkref #1: rel
1663/5/16397 blk 0
rmgr: Transaction len (rec/tot):     46/    46, tx:        720, lsn:
0/01FFC200, prev 0/01FFC1B8, desc: COMMIT 2026-03-29 10:15:24.112967
NZDT
pg_waldump: error: could not find WAL "000000010000000000000002" in
archive "_tmp_H_1gv81G1L_pg_wal.tar"

Seems like it already stepped over 000000010000000000000002 earlier?
Could it be a table-of-contents order dependency bug or something like
that?

---

From: Thomas Munro <[email-masked]>
Date: Sun, 29 Mar 2026 11:08:43 +1300

On Sun, Mar 29, 2026 at 10:49 AM Tom Lane <[email-masked]> wrote:
> Huh.  What is your platform exactly?  Maybe more to the point,
> what is the tar program you're using?

FreeBSD tar 3.8.2.

> If you look at the TAP script, you'll see that it tries to randomize
> the order of the entries in the tar file (see sub generate_archive).
> So if that's the problem, it shouldn't reproduce 100%, and also we
> should be seeing lots of freckles on the buildfarm.  We're not, so
> there must be something off-the-beaten-track about your test
> environment.

Right, I see now.  There is something different about
000000010000000000000002 though: it doesn't seem to have a normal
(non-PAX, non-GNU) TOC entry, unlike000000010000000000000001.  Trying
to figure out why...

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 28 Mar 2026 23:57:37 +0100

On 3/28/26 23:36, Tom Lane wrote:
> I wrote:
>> However ... I do not find any indication in the GNU tar docs
>> that it produces sparse files by default.  It looks like you
>> need to say -S/--sparse to make that happen.  Maybe you have
>> a version that's been hacked to make that the default?
> 
> Bleah.  Digging in the man pages at freebsd.org, I read
> 
>     --read-sparse
> 	       (c, r, u	modes only) Read sparse	file  information  from	 disk.
> 	       This  is	the reverse of --no-read-sparse	and the	default	behav-
> 	       ior.
> 
> It's apparently been there and been default since FreeBSD 13.1.
> This leads one to wonder how come BF member dikkop is managing
> to run this test successfully.  I speculate that it's using a
> filesystem type that doesn't do sparse files (cc'ing Vondra
> for confirmation on that).
> 

It's running on ufs. But I think the explanation is very simple. We had
a short power outage on Thursday, and the FreeBSD machine failed to boot
properly after the power was restored. IIUC this test is new, right?

I fixed the machine, it'll start running the tests in a couple minutes.

regards

-- 
Tomas Vondra

---

From: Thomas Munro <[email-masked]>
Date: Sun, 29 Mar 2026 12:15:39 +1300

On Sun, Mar 29, 2026 at 11:37 AM Tom Lane <[email-masked]> wrote:
> It looks like to make this test stable on modern FreeBSD,
> we need to see if tar accepts --no-read-sparse and use that
> switch if so.

Yeah.  Here's my attempt at perl.

I think your Mac probably has a similar tar program BTW... but apfs
probably doesn't go around making holes visible to lseek()
automatically or at least as eagerly as my ZFS system.
From b1a9131d072dadfe9c5666e75a932ba70b8d2f99 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Sun, 29 Mar 2026 12:03:42 +1300
Subject: [PATCH] Fix pg_waldump test for libarchive tar.

libarchive tar (the one shipped on macOS, *BSD systems) might decide to
archive non-standard sparse encoding with GNU extensions (unlike GNU tar
itself) by default.  pg_waldump can't read them.  Suppress that, if $TAR
understands --no-sparse-files.

Discussion: https://postgr.es/m/1624716.1774736283%40sss.pgh.pa.us
---
 src/bin/pg_waldump/t/001_basic.pl | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index 8bb8fa225f6..d911296bb66 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -11,6 +11,14 @@ use Test::More;
 use List::Util qw(shuffle);
 
 my $tar = $ENV{TAR};
+my @TAR_C_FLAGS;
+
+# libarchive tar (as found on *BSD and macOS) might create sparse files by
+# default, and we can't read them
+if (system("$tar --no-read-sparse -c - /dev/null > /dev/null") == 0)
+{
+  push(@TAR_C_FLAGS, "--no-read-sparse");
+}
 
 program_help_ok('pg_waldump');
 program_version_ok('pg_waldump');
@@ -331,7 +339,6 @@ sub test_pg_waldump
 sub generate_archive
 {
 	my ($archive, $directory, $compression_flags) = @_;
-
 	my @files;
 	opendir my $dh, $directory or die "opendir: $!";
 	while (my $entry = readdir $dh) {
@@ -346,7 +353,7 @@ sub generate_archive
 	# move into the WAL directory before archiving files
 	my $cwd = getcwd;
 	chdir($directory) || die "chdir: $!";
-	command_ok([$tar, $compression_flags, $archive, @files]);
+	command_ok([$tar, @TAR_C_FLAGS, $compression_flags, $archive, @files]);
 	chdir($cwd) || die "chdir: $!";
 }
 
-- 
2.52.0

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 29 Mar 2026 15:33:10 +0200

On 3/29/26 00:12, Tom Lane wrote:
> Tomas Vondra <[email-masked]> writes:
> Not that new, it dates to b15c15139, about a week ago.
> 
> I've reproduced Thomas' failure on a local FreeBSD 15.0 image
> using zfs, and confirmed that this cowboy hack fixes it:
> 

Interesting. Then I guess it has to be due to some difference in ufs vs.
zfs, when handling sparse files. It might be useful to add a bit more
variation here, and switch some of the animals to non-default
filesystems (not just the FreeBSD ones, which we seem to have only two
that run reasonably often). I'd bet most of the linux systems run on
ext4/xfs, few on btrfs/zfs.

regards

-- 
Tomas Vondra

---

From: Thomas Munro <[email-masked]>
Date: Mon, 30 Mar 2026 11:11:50 +1300

> ext4/xfs, few on btrfs/zfs.

UFS does have sparse files (its ancestor invented them some time
around (time_t) 0), it just doesn't make them unless you tell it to.
PostgreSQL only does that if you set wal_init_zero=false.

ZFS is different because it creates holes automagically when you write
zeroes, at least if compression is enabled so it has to scan all your
bytes anyway.

I was curious to know if BTRFS does that too, or hides
zero-compression at some lower invisible level:

$ echo "hello" > 1MB-sparse.dat
$ truncate -s 512KB 1MB-sparse.dat
$ echo "world" >> 1MB-sparse.dat
$ truncate -s 1MB 1MB-sparse.dat
$ ls -l 1MB-sparse.dat
-rw-rw-r-- 1 tmunro tmunro 1000000 Mar 30 10:11 1MB-sparse.dat
$ du -hs 1MB-sparse.dat
8.0K    1MB-sparse.dat
$ strace tar -S -cf foo.tar 1MB-sparse.dat 2>&1 | grep seek
lseek(4, 0, SEEK_DATA)                  = 0
lseek(4, 0, SEEK_HOLE)                  = 4096
lseek(4, 4096, SEEK_DATA)               = 512000
lseek(4, 512000, SEEK_HOLE)             = 516096
lseek(4, 516096, SEEK_DATA)             = -1 ENXIO (No such device or address)

... so that's a yes, lseek sees holes that we didn't ask it to make,
just like on ZFS, but the rest of this trace of GNU tar -S -cf is
interesting:

lseek(5, 0, SEEK_SET)                   = 0
lseek(5, 0, SEEK_SET)                   = 0
lseek(4, 0, SEEK_SET)                   = 0
lseek(4, 512000, SEEK_SET)              = 512000
lseek(4, 1000000, SEEK_SET)             = 1000000

It didn't write out PAX format!  Instead it replicated the holes into
the tar file itself with SEEK_SET.

$ strings foo.tar | grep Sparse

You have to add --format=posix to enable the GNU behaviour that BSD
tar is emulating by default:

$ tar --format=posix -S -cf foo.tar 1MB-sparse.dat
$ strings foo.tar | grep Sparse
./GNUSparseFile.4190/1MB-sparse.dat

I expected GNU tar to be forced to do that if writing to non-seekable
output, eg "tar -S -c 1MB-sparse.dat | cat > foo.tar", but somehow it
manages to write out only ~10KB of plain ustar format that it is able
to restore to the full 1MB apparent size using some other trick, but
... ENOTIME, I dunno how it's doing that.  Might be interesting to see
if pg_waldump can read it though, 'cause the bytes aren't all there.

BTW I confirmed that Apple tar does have -S by default too, it's just
that APFS doesn't make holes magically, so this test would presumably
have broken on a Mac if wal_init_zero had been forced to zero (not
tested).

Anyway, given the defaults, GNU tar + ZFS/BTRFS users must be pretty
unlikely to hit this in the wild, and the symptom is a confusing error
in a maintenance tool, not corruption, so I don't think this is a big
deal.  I might still try teaching the astreamer code to understand PAX
1.0 when it sees it in the next cycle though, for the benefit of
FreeBSD users.  A quick and dirty version could probably just unmangle
the name and skip the first block of data, since any valid WAL file
will not begin with a hole and valid WAL data will end at the first
hole and fail our verification, but of course a real implementation
should read the map properly[1]...

[1] https://www.gnu.org/software/tar/manual/html_node/PAX-1.html

---

From: Thomas Munro <[email-masked]>
Date: Mon, 30 Mar 2026 11:20:24 +1300

> ... so that's a yes, lseek sees holes that we didn't ask it to make,

Oops, sorry, I wrote that email too fast and got my examples mixed up,
BTFS actually *doesnt* do that automatically, that was of course a
trace showing a file with explicitly made holes.  So this is probably
be a ZFS-only issue unless you're using wal_init_zero=0, and then any
file system could result in PAX-sparse-format tarballs, but even then
only if you use non-default switches that in practice no one will use
with GNU tar, or if you use BSD tar.  So in practice this is a
FreeBSD-only issue.

---

From: Thomas Munro <[email-masked]>
Date: Wed, 1 Apr 2026 15:05:53 +1300

On Mon, Mar 30, 2026 at 11:23 AM Tom Lane <[email-masked]> wrote:
> Thomas Munro <[email-masked]> writes:
> > FreeBSD users.
> I agree that this isn't too critical if the effects are confined to
> pg_waldump.  I believe that pg_basebackup and pg_verifybackup also use
> astreamer_tar.c, but it's not clear to me if they'd ever be asked to
> parse files made by tar(1) and not by our own sparseness-ignorant
> tar-writing code.  If they can be, that'd be a higher-priority reason
> to fill in this gap.

I pushed the workaround for the test.

Yeah I can't see any reason why pg_verifybackup --wal-path=foo.tar
won't suffer the same problem in the wild.  Again, it's not the end of
the world because it'll just fail and you'll probably eventually
figure out why.  So perhaps we should just improve our detection of
archives that we can't handle?  Straw man algorithm:

If you can't find $NAME in the archive, then check if PaxHeaders/$NAME
exists, and if so, fail with 'unsupported TAR format for WAL file "%s"
in archive "%s"' instead.  That'd probably work well enough in
practice, because astreamer_tar.c treats PAX extended header
pseudo-files as regular files (they're not, they have type 'x'), and
both GNU and BSD tar happen to use that.

POSIX doesn't require that naming, so it would in theory be more
correct to teach astreamer_tar.c to recognise PAX extended headers and
fish out enough information and link it to the following archive
member, but a simple test to improve error messaging seems like the
right level of effort here.

Here's a test patch that shows the problem on any system with GNU tar
or BSD tar and a file system that supports sparse files.  The test
succeeds because it looks for "error: could not find WAL" but the idea
would be to change it to look for a new error message like that.  My
motivation was to make this reproducible on any system, in case that's
helpful for Amul and Andrew if they're interested in trying to improve
this edge case in time for the release.  Otherwise I'll come back to
it, but probably not in time...
From 084d71f81143f0462caf03569722b5f0b2a147e6 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Mon, 30 Mar 2026 18:20:09 +1300
Subject: [PATCH] Add a pg_waldump test with GNU tar PAX format.

XXX Update this to test for a new improved error message!

XXX Should this test run for all the scenarios?  Doesn't seem like
compression is relevant to this problem so I just added it as a
standalone test...

XXX No doubt the perl isn't the greatest...
---
 src/bin/pg_waldump/t/001_basic.pl | 73 +++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index ce1f6aa30c0..7f8a319c85d 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -6,6 +6,7 @@ use warnings FATAL => 'all';
 use Cwd;
 use File::Copy;
 use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::RecursiveCopy;
 use PostgreSQL::Test::Utils;
 use Test::More;
 use List::Util qw(shuffle);
@@ -212,9 +213,13 @@ $node->safe_psql('postgres',
 	qq{SELECT pg_logical_emit_message(true, 'test 026', repeat('xyzxz', 123456))}
 );
 
-my ($end_lsn, $end_walfile) = split /\|/,
+my ($end_lsn, $end_walfile, $wal_segsize) = split /\|/,
   $node->safe_psql('postgres',
-	q{SELECT pg_current_wal_insert_lsn(), pg_walfile_name(pg_current_wal_insert_lsn())}
+	q{SELECT pg_current_wal_insert_lsn(),
+			 pg_walfile_name(pg_current_wal_insert_lsn()),
+			 setting
+        FROM pg_settings
+       WHERE name = 'wal_segment_size'}
   );
 
 my $default_ts_oid = $node->safe_psql('postgres',
@@ -339,7 +344,7 @@ sub test_pg_waldump
 # Create a tar archive, shuffle the file order
 sub generate_archive
 {
-	my ($archive, $directory, $compression_flags) = @_;
+	my ($archive, $directory, $compression_flags, @extra_flags) = @_;
 
 	my @files;
 	opendir my $dh, $directory or die "opendir: $!";
@@ -350,12 +355,17 @@ sub generate_archive
 	}
 	closedir $dh;
 
+	if (!@extra_flags)
+	{
+		@extra_flags = @tar_c_flags;
+	}
+
 	@files = shuffle @files;
 
 	# move into the WAL directory before archiving files
 	my $cwd = getcwd;
 	chdir($directory) || die "chdir: $!";
-	command_ok([$tar, @tar_c_flags, $compression_flags, $archive, @files]);
+	command_ok([$tar, @extra_flags, $compression_flags, $archive, @files]);
 	chdir($cwd) || die "chdir: $!";
 }
 
@@ -477,4 +487,59 @@ for my $scenario (@scenarios)
 	}
 }
 
+SKIP:
+	skip "tar command is not available", 1
+		if !defined $tar;
+
+	my @sparse_flags;
+
+	# Tell $TAR to use GNU tar's PAX sparse file archive format, so we can test
+	# our handling of that.
+
+	# GNU tar
+	@sparse_flags = ("--sparse", "--format=pax")
+		if system("$tar --sparse --format=pax -c " .
+				  $node->data_dir . "/pg_wal/* /dev/null > /dev/null") == 0;
+	# BSD tar (this is the default, but we still need to detect BSD tar)
+	@sparse_flags = ("--read-sparse", "--format=pax")
+		if system("$tar --read-sparse --format=pax -c " .
+				  $node->data_dir . "/pg_wal/* /dev/null > /dev/null") == 0;
+
+	skip "tar command doesn't support GNU PAX format for sparse files", 1
+		if !@sparse_flags;
+
+	PostgreSQL::Test::RecursiveCopy::copypath($node->data_dir . '/pg_wal',
+											  $tmp_dir . '/pg_wal_sparse');
+
+	# truncate the unused part of final WAL file
+	my $end_byte = $end_lsn;
+	$end_byte =~ s/\///;
+	$end_byte = hex($end_byte);
+	$end_byte %= $wal_segsize;
+	truncate $tmp_dir . '/pg_wal_sparse/' . $end_walfile, $end_byte;
+
+	# now re-extend it to create a hole
+	truncate $tmp_dir . '/pg_wal_sparse/' . $end_walfile, $wal_segsize;
+
+	# XXX maybe we should detect sparse files with stat (size > blocks * block
+	# size?), and skip the test if truncate failed to make one... that
+	# might happen on eg windows I think?  otherwise we'd have to tolerate
+	# the pg_waldump command succeeding OR failing with a certain message
+
+	generate_archive($tmp_dir . '/pg_wal_sparse.tar',
+					 $tmp_dir . '/pg_wal_sparse',
+					 '-cf',
+					 @sparse_flags);
+
+	# XXX change this to check for new improved error message
+	command_fails_like(
+		[
+			'pg_waldump',
+			'--path' => $tmp_dir . '/pg_wal_sparse.tar',
+			'--start' => $start_lsn,
+			'--end' => $end_lsn,
+		],
+		qr/error: could not find WAL/,
+		'fails with GNU tar PAX-format sparse files');
+
 done_testing();
-- 
2.53.0

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 1 Apr 2026 06:39:05 -0400

On 2026-03-31 Tu 10:05 PM, Thomas Munro wrote:
> On Mon, Mar 30, 2026 at 11:23 AM Tom Lane<[email-masked]> wrote:
>> Thomas Munro<[email-masked]> writes:

It occurred to me this morning that we probably shouldn't run this test 
on Windows, and if we do we shouldn't be using /dev/null (the Windows 
equivalent of which is just "nul"). The simplest fix would just be to 
add a "!$windows_os" to the if test.

cheers

andrew

---

From: Andres Freund <[email-masked]>
Date: Wed, 1 Apr 2026 09:26:00 -0400

Hi,

On 2026-04-01 06:39:05 -0400, Andrew Dunstan wrote:
> It occurred to me this morning that we probably shouldn't run this test on
> Windows, and if we do we shouldn't be using /dev/null (the Windows
> equivalent of which is just "nul"). The simplest fix would just be to add a
> "!$windows_os" to the if test.

Why should we skip this test on windows?

I think we have historically been way too liberal about sprinkling
!$windows_os test disablements around. More than once there were actual bugs
that we just swept under the rug by disabling the tests that detected them.
Either we support windows or we don't.

Greetings,

Andres Freund

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 1 Apr 2026 10:19:32 -0400

> Either we support windows or we don't.

Maybe I misunderstood, but I didn't think this was going to be an issue 
on NTFS.

In general I agree with you, though. I try to avoid skipping things on 
Windows.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Wed, 01 Apr 2026 14:25:40 -0400

> archives that we can't handle?

After reading the POSIX spec for pax format (in the pax(1) man page),
I think it's absolutely essential that we reject files that contain
pax extension headers.  Those can change the interpretation of the
following file header(s) in nearly arbitrary ways, so we have plenty
of problems besides this sparse-file issue if we just ignore them.

(Of course, later we can consider improving the code to handle them
correctly, but that ain't happening in time for v19.)

Also, if we are admitting the possibility that what we are reading
was made by a platform-supplied tar and not our own code, I think
it verges on lunacy to behave as though unsupported typeflags are
regular files.

So I think we need something more or less like the attached.

			regards, tom lane

diff --git a/src/bin/pg_basebackup/astreamer_inject.c b/src/bin/pg_basebackup/astreamer_inject.c
index 14a96733bad..a5dff0ac1c6 100644
--- a/src/bin/pg_basebackup/astreamer_inject.c
+++ b/src/bin/pg_basebackup/astreamer_inject.c
@@ -224,8 +224,9 @@ astreamer_inject_file(astreamer *streamer, char *pathname, char *data,
 	strlcpy(member.pathname, pathname, MAXPGPATH);
 	member.size = len;
 	member.mode = pg_file_create_mode;
+	member.is_regular = true;
 	member.is_directory = false;
-	member.is_link = false;
+	member.is_symlink = false;
 	member.linktarget[0] = '\0';
 
 	/*
diff --git a/src/bin/pg_verifybackup/astreamer_verify.c b/src/bin/pg_verifybackup/astreamer_verify.c
index 26c98186530..99bd69ce7c9 100644
--- a/src/bin/pg_verifybackup/astreamer_verify.c
+++ b/src/bin/pg_verifybackup/astreamer_verify.c
@@ -165,7 +165,7 @@ member_verify_header(astreamer *streamer, astreamer_member *member)
 	char		pathname[MAXPGPATH];
 
 	/* We are only interested in normal files. */
-	if (member->is_directory || member->is_link)
+	if (!member->is_regular)
 		return;
 
 	/*
diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index ff1cc3fa5f0..e4a4bf44a7e 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -815,7 +815,7 @@ member_is_wal_file(astreamer_waldump *mystreamer, astreamer_member *member,
 	char	   *filename;
 
 	/* We are only interested in normal files */
-	if (member->is_directory || member->is_link)
+	if (!member->is_regular)
 		return false;
 
 	if (strlen(member->pathname) < XLOG_FNAME_LEN)
diff --git a/src/fe_utils/astreamer_file.c b/src/fe_utils/astreamer_file.c
index 158e9a14f2c..0fca70a4f86 100644
--- a/src/fe_utils/astreamer_file.c
+++ b/src/fe_utils/astreamer_file.c
@@ -228,9 +228,13 @@ astreamer_extractor_content(astreamer *streamer, astreamer_member *member,
 				mystreamer->filename[fnamelen - 1] = '\0';
 
 			/* Dispatch based on file type. */
-			if (member->is_directory)
+			if (member->is_regular)
+				mystreamer->file =
+					create_file_for_extract(mystreamer->filename,
+											member->mode);
+			else if (member->is_directory)
 				extract_directory(mystreamer->filename, member->mode);
-			else if (member->is_link)
+			else if (member->is_symlink)
 			{
 				const char *linktarget = member->linktarget;
 
@@ -238,10 +242,6 @@ astreamer_extractor_content(astreamer *streamer, astreamer_member *member,
 					linktarget = mystreamer->link_map(linktarget);
 				extract_link(mystreamer->filename, linktarget);
 			}
-			else
-				mystreamer->file =
-					create_file_for_extract(mystreamer->filename,
-											member->mode);
 
 			/* Report output file change. */
 			if (mystreamer->report_output_file)
diff --git a/src/fe_utils/astreamer_tar.c b/src/fe_utils/astreamer_tar.c
index 3b094fc0328..3de6f8d6fb2 100644
--- a/src/fe_utils/astreamer_tar.c
+++ b/src/fe_utils/astreamer_tar.c
@@ -272,6 +272,9 @@ astreamer_tar_header(astreamer_tar_parser *mystreamer)
 
 	Assert(mystreamer->base.bbs_buffer.len == TAR_BLOCK_SIZE);
 
+	/* Zero out fields of *member, just for consistency. */
+	memset(member, 0, sizeof(astreamer_member));
+
 	/* Check whether we've got a block of all zero bytes. */
 	for (i = 0; i < TAR_BLOCK_SIZE; ++i)
 	{
@@ -299,12 +302,28 @@ astreamer_tar_header(astreamer_tar_parser *mystreamer)
 	member->mode = read_tar_number(&buffer[TAR_OFFSET_MODE], 8);
 	member->uid = read_tar_number(&buffer[TAR_OFFSET_UID], 8);
 	member->gid = read_tar_number(&buffer[TAR_OFFSET_GID], 8);
-	member->is_directory =
-		(buffer[TAR_OFFSET_TYPEFLAG] == TAR_FILETYPE_DIRECTORY);
-	member->is_link =
-		(buffer[TAR_OFFSET_TYPEFLAG] == TAR_FILETYPE_SYMLINK);
-	if (member->is_link)
-		strlcpy(member->linktarget, &buffer[TAR_OFFSET_LINKNAME], 100);
+
+	switch (buffer[TAR_OFFSET_TYPEFLAG])
+	{
+		case TAR_FILETYPE_PLAIN:
+		case '\0':				/* backwards compatibility hack, per POSIX */
+			member->is_regular = true;
+			break;
+		case TAR_FILETYPE_DIRECTORY:
+			member->is_directory = true;
+			break;
+		case TAR_FILETYPE_SYMLINK:
+			member->is_symlink = true;
+			strlcpy(member->linktarget, &buffer[TAR_OFFSET_LINKNAME], 100);
+			break;
+		case TAR_FILETYPE_PAX_EXTENDED:
+		case TAR_FILETYPE_PAX_EXTENDED_GLOBAL:
+			pg_fatal("pax extensions to tar format are not supported");
+			break;
+		default:
+			/* For special files, set none of the three is_xxx flags */
+			break;
+	}
 
 	/* Compute number of padding bytes. */
 	mystreamer->pad_bytes_expected = tarPaddingBytesRequired(member->size);
diff --git a/src/include/fe_utils/astreamer.h b/src/include/fe_utils/astreamer.h
index f370ef62720..8329e4efbc5 100644
--- a/src/include/fe_utils/astreamer.h
+++ b/src/include/fe_utils/astreamer.h
@@ -83,8 +83,10 @@ typedef struct
 	mode_t		mode;
 	uid_t		uid;
 	gid_t		gid;
+	/* note: special filetypes will set none of these flags */
+	bool		is_regular;
 	bool		is_directory;
-	bool		is_link;
+	bool		is_symlink;
 	char		linktarget[MAXPGPATH];
 } astreamer_member;
 
diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index eb93bdef5c4..d6c434e09b0 100644
--- a/src/include/pgtar.h
+++ b/src/include/pgtar.h
@@ -60,6 +60,8 @@ enum tarFileType
 	TAR_FILETYPE_PLAIN = '0',
 	TAR_FILETYPE_SYMLINK = '2',
 	TAR_FILETYPE_DIRECTORY = '5',
+	TAR_FILETYPE_PAX_EXTENDED = 'x',
+	TAR_FILETYPE_PAX_EXTENDED_GLOBAL = 'g',
 };
 
 extern enum tarError tarCreateHeader(char *h, const char *filename,

---

From: Thomas Munro <[email-masked]>
Date: Thu, 2 Apr 2026 13:16:17 +1300

> it verges on lunacy to behave as though unsupported typeflags are

Yeah, if this is the first time we parse files we didn't make then
that makes total sense.  I was a bit unsure of that question when I
suggested we reject pax only after we've failed to find a file, in
case there are scenarios that work today with harmless ignorable pax
headers that don't change the file name.

> So I think we need something more or less like the attached.

LGTM.  Tested with both tars here.  I updated that little test patch
for this.  Not sure if you think it's worth a test though, now that
it's so simple.

@Andrew: I tried using File::Spec->devnull() this time.  Are you able
to check if this works OK on Windows, applied on top of Tom's patch?
AFAIK should be able to run this new test and pass, not skip it.  But
it could be that the shell invocation needs tweaking.  It's hard to
tell from CI.  (Huh, apparently Windows ships a copy of BSD tar as
C:\Windows\System32\tar.exe these days.)
From 57b9d1a58a9eb30feec72b0ccce6bdfd5e1ef92e Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Mon, 30 Mar 2026 18:20:09 +1300
Subject: [PATCH] Test rejection of pax extended tar files.

Also change 852de579's shell invocation to use NUL on Windows instead of
/dev/null, per feedback from Andrew Dunstan.  That command wasn't
expected to succeed on Windows, but the similar usage added here might.
---
 src/bin/pg_waldump/t/001_basic.pl | 42 +++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_waldump/t/001_basic.pl b/src/bin/pg_waldump/t/001_basic.pl
index ce1f6aa30c0..3b99a8f6d4f 100644
--- a/src/bin/pg_waldump/t/001_basic.pl
+++ b/src/bin/pg_waldump/t/001_basic.pl
@@ -5,6 +5,7 @@ use strict;
 use warnings FATAL => 'all';
 use Cwd;
 use File::Copy;
+use File::Spec;
 use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
@@ -13,12 +14,14 @@ use List::Util qw(shuffle);
 my $tar = $ENV{TAR};
 my @tar_c_flags;
 
-# By default, bsdtar archives sparse files in GNU tar's --format=posix --sparse
-# format, so pg_waldump can't find files that ZFS has decided to store with
-# holes.  Turn that off.
-if (system("$tar --no-read-sparse -c - /dev/null > /dev/null") == 0)
+my $devnull = File::Spec->devnull();
+
+# By default, bsdtar archives sparse files in GNU tar's --format=pax --sparse
+# format, so pg_waldump rejects WAL that ZFS has decided to store with holes.
+# Turn that off.
+if (system("$tar --no-read-sparse -c $devnull > $devnull") == 0)
 {
-  push(@tar_c_flags, "--no-read-sparse");
+	push(@tar_c_flags, "--no-read-sparse");
 }
 
@@ -339,7 +342,7 @@ sub test_pg_waldump
 
 	my @files;
 	opendir my $dh, $directory or die "opendir: $!";
@@ -355,7 +358,7 @@ sub generate_archive
 	# move into the WAL directory before archiving files
 	my $cwd = getcwd;
 	chdir($directory) || die "chdir: $!";
-	command_ok([$tar, @tar_c_flags, $compression_flags, $archive, @files]);
+	command_ok([$tar, @extra_flags, @tar_c_flags, $compression_flags, $archive, @files]);
 	chdir($cwd) || die "chdir: $!";
 }
 
@@ -477,4 +480,29 @@ for my $scenario (@scenarios)
 	}
 }
 
+SKIP:
+{
+	skip "tar command is not available", 1
+		if !defined $tar;
+
+	skip "tar command doesn't understand --format=pax", 1
+		if system("$tar --format=pax -c " .
+				  $node->data_dir . "/pg_wal/* > $devnull") != 0;
+
+	generate_archive($tmp_dir . '/pg_wal_pax.tar',
+					 $node->data_dir . '/pg_wal',
+					 '-cf',
+					 ("--format=pax"));
+
+	command_fails_like(
+		[
+			'pg_waldump',
+			'--path' => $tmp_dir . '/pg_wal_pax.tar',
+			'--start' => $start_lsn,
+			'--end' => $end_lsn,
+		],
+		qr/error: pax extensions to tar format are not supported/,
+		'fails if pax extended header is detected');
+}
+
 done_testing();
-- 
2.52.0

---

From: Tom Lane <[email-masked]>
Date: Wed, 01 Apr 2026 21:22:44 -0400

> headers that don't change the file name.

Of course this is all new so far as pg_waldump is concerned.
I'm a bit unclear about whether pg_verifybackup's exposure
is large enough to warrant back-patching any of this.

Looking again at astreamer_tar.c, I suddenly realized that it doesn't
do any meaningful input validation.  So if you feed it junk input,
you get garbage errors that aren't even predictable:

$ head -c 32768 /dev/urandom >junk.tar
$ pg_waldump --path junk.tar --start 1/0x10000000
pg_waldump: error: COPY stream ended before last file was finished
$ head -c 32768 /dev/urandom >junk.tar
$ pg_waldump --path junk.tar --start 1/0x10000000
pg_waldump: error: tar member has empty name
$ head -c 32768 /dev/urandom >junk.tar
$ pg_waldump --path junk.tar --start 1/0x10000000
pg_waldump: error: COPY stream ended before last file was finished
$ head -c 32768 /dev/urandom >junk.tar
$ pg_waldump --path junk.tar --start 1/0x10000000
pg_waldump: error: could not find WAL in archive "junk.tar"

tar itself is considerably saner:

$ tar tf junk.tar 
tar: This does not look like a tar archive
tar: Skipping to next header
tar: Exiting with failure status due to previous errors

So I think we need something like the attached, in addition
to what I sent before.  This just makes astreamer_tar.c use
the isValidTarHeader function that pg_dump already had.
(I decided to const-ify isValidTarHeader's argument while
moving it to a shared location, which in turn requires
const-ifying tarChecksum.)

			regards, tom lane

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 271a2c3e481..fecf6f2d1ce 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -44,6 +44,7 @@
 #include "pg_backup_archiver.h"
 #include "pg_backup_db.h"
 #include "pg_backup_utils.h"
+#include "pgtar.h"
 
 #define TEXT_DUMP_HEADER "--\n-- PostgreSQL database dump\n--\n\n"
 #define TEXT_DUMPALL_HEADER "--\n-- PostgreSQL database cluster dump\n--\n\n"
@@ -2372,7 +2373,7 @@ _discoverArchiveFormat(ArchiveHandle *AH)
 		}
 
 		if (!isValidTarHeader(AH->lookahead))
-			pg_fatal("input file does not appear to be a valid archive");
+			pg_fatal("input file does not appear to be a valid tar archive");
 
 		AH->format = archTar;
 	}
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index 365073b3eae..9c3aca6543a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -465,8 +465,6 @@ extern void InitArchiveFmt_Null(ArchiveHandle *AH);
 extern void InitArchiveFmt_Directory(ArchiveHandle *AH);
 extern void InitArchiveFmt_Tar(ArchiveHandle *AH);
 
-extern bool isValidTarHeader(char *header);
-
 extern void ReconnectToServer(ArchiveHandle *AH, const char *dbname);
 extern void IssueCommandPerBlob(ArchiveHandle *AH, TocEntry *te,
 								const char *cmdBegin, const char *cmdEnd);
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index d94d0de2a5d..a3879410c94 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -984,31 +984,6 @@ tarPrintf(TAR_MEMBER *th, const char *fmt,...)
 	return (int) cnt;
 }
 
-bool
-isValidTarHeader(char *header)
-{
-	int			sum;
-	int			chk = tarChecksum(header);
-
-	sum = read_tar_number(&header[TAR_OFFSET_CHECKSUM], 8);
-
-	if (sum != chk)
-		return false;
-
-	/* POSIX tar format */
-	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar\0", 6) == 0 &&
-		memcmp(&header[TAR_OFFSET_VERSION], "00", 2) == 0)
-		return true;
-	/* GNU tar format */
-	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar  \0", 8) == 0)
-		return true;
-	/* not-quite-POSIX format written by pre-9.3 pg_dump */
-	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar00\0", 8) == 0)
-		return true;
-
-	return false;
-}
-
 /* Given the member, write the TAR header & copy the file */
 static void
 _tarAddFile(ArchiveHandle *AH, TAR_MEMBER *th)
diff --git a/src/fe_utils/astreamer_tar.c b/src/fe_utils/astreamer_tar.c
index 3b094fc0328..8fa3329237d 100644
--- a/src/fe_utils/astreamer_tar.c
+++ b/src/fe_utils/astreamer_tar.c
@@ -260,7 +260,8 @@ astreamer_tar_parser_content(astreamer *streamer, astreamer_member *member,
  * Parse a file header within a tar stream.
  *
  * The return value is true if we found a file header and passed it on to the
- * next astreamer; it is false if we have reached the archive trailer.
+ * next astreamer; it is false if we have found the archive trailer.
+ * We throw error if we see invalid data.
  */
 static bool
 astreamer_tar_header(astreamer_tar_parser *mystreamer)
@@ -289,6 +290,12 @@ astreamer_tar_header(astreamer_tar_parser *mystreamer)
 	if (!has_nonzero_byte)
 		return false;
 
+	/*
+	 * Verify that we have a reasonable-looking header.
+	 */
+	if (!isValidTarHeader(buffer))
+		pg_fatal("input file does not appear to be a valid tar archive");
+
 	/*
 	 * Parse key fields out of the header.
 	 */
diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index eb93bdef5c4..55b8c7c77a4 100644
--- a/src/include/pgtar.h
+++ b/src/include/pgtar.h
@@ -68,7 +68,8 @@ extern enum tarError tarCreateHeader(char *h, const char *filename,
 									 time_t mtime);
 extern uint64 read_tar_number(const char *s, int len);
 extern void print_tar_number(char *s, int len, uint64 val);
-extern int	tarChecksum(char *header);
+extern int	tarChecksum(const char *header);
+extern bool isValidTarHeader(const char *header);
 
 /*
  * Compute the number of padding bytes required for an entry in a tar
diff --git a/src/port/tar.c b/src/port/tar.c
index 592b4fb7b0f..db462b90292 100644
--- a/src/port/tar.c
+++ b/src/port/tar.c
@@ -87,7 +87,7 @@ read_tar_number(const char *s, int len)
  * be 512 bytes, per the tar standard.
  */
 int
-tarChecksum(char *header)
+tarChecksum(const char *header)
 {
 	int			i,
 				sum;
@@ -104,6 +104,35 @@ tarChecksum(char *header)
 	return sum;
 }
 
+/*
+ * Check validity of a tar header (assumed to be 512 bytes long).
+ * We verify the checksum and the magic number / version.
+ */
+bool
+isValidTarHeader(const char *header)
+{
+	int			sum;
+	int			chk = tarChecksum(header);
+
+	sum = read_tar_number(&header[TAR_OFFSET_CHECKSUM], 8);
+
+	if (sum != chk)
+		return false;
+
+	/* POSIX tar format */
+	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar\0", 6) == 0 &&
+		memcmp(&header[TAR_OFFSET_VERSION], "00", 2) == 0)
+		return true;
+	/* GNU tar format */
+	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar  \0", 8) == 0)
+		return true;
+	/* not-quite-POSIX format written by pre-9.3 pg_dump */
+	if (memcmp(&header[TAR_OFFSET_MAGIC], "ustar00\0", 8) == 0)
+		return true;
+
+	return false;
+}
+
 
 /*
  * Fill in the buffer pointed to by h with a tar format header. This buffer

---

From: Thomas Munro <[email-masked]>
Date: Thu, 2 Apr 2026 15:14:09 +1300

> you get garbage errors that aren't even predictable:

Wow.

> const-ifying tarChecksum.)

LGTM.

$ echo -n x | dd of=foo.tar bs=1 seek=257 count=1 conv=notrunc
$ strings foo.tar | grep tar | head -1
xstar
$ pg_waldump --path=foo.tar -s 0/1 -e 0/100
pg_waldump: error: input file does not appear to be a valid tar archive

$ echo -n u | dd of=foo.tar bs=1 seek=257 count=1 conv=notrunc
$ strings foo.tar | grep tar | head -1
ustar
$ pg_waldump --path=foo.tar -s 0/1 -e 0/100
... other output...

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 2 Apr 2026 07:36:25 -0400

On 2026-04-01 We 8:16 PM, Thomas Munro wrote:
> On Thu, Apr 2, 2026 at 7:25 AM Tom Lane<[email-masked]> wrote:
> @Andrew: I tried usingFile::Spec->devnull() this time.  Are you able

Yes, that appears to work. I would put a "2>&1" at the end - we don't 
care about the output, just whether or not it succeeds:

C:\Windows\system32>perl -MFile::Spec -e "print File::Spec->devnull();"
nul

C:\Windows\system32>tar --no-read-sparse -c - nul > nul 2>&1 && echo hello

C:\Windows\system32>tar --no-read-sparse -c - nul > nul 2>&1 || echo hello
hello

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 12:29:52 -0400

> LGTM.

Pushed, thanks for reviewing!  In the event I decided to back-patch to
v18, where these fixes could protect pg_verifybackup against tar files
it can't handle.  In older branches the astreamer (nee bbstreamer)
logic only exists inside pg_basebackup, and IIUC that's not really
exposed to tar files that weren't made by our own code.  So I did
not bother with back-patching further, though it'd be possible to
do that if someone knows a scenario where it'd matter.

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 2 Apr 2026 19:13:08 +0200

> do that if someone knows a scenario where it'd matter.

It seems jay/hippopotamus failed on this, for some reason. Those two
animals are on the same host, just using different compilers. I did
update+reboot the machine this afternoon (before the test), but I don't
see why that would cause the failure.

Maybe there's something special about OpenSUSE?

-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 13:43:36 -0400

Tomas Vondra <[email-masked]> writes:
>> it can't handle.

> It seems jay/hippopotamus failed on this, for some reason.

Hmm:

#   Failed test 'corrupt backup fails verification: extra_file: matches'
#   at t/003_corruption.pl line 198.
#                   'pg_verifybackup: error: pax extensions to tar format are not supported
# '
#     doesn't match '(?^:extra_file.*present (on disk|in archive "[^"]+") but not in the manifest)'

> Maybe there's something special about OpenSUSE?

Apparently its version of "tar" will produce pax-extended files
at the drop of a hat.  I have an OpenSUSE image around here
somewhere, will see if I can reproduce this.  But while I'm
asking, what filesystem are those animals running on top of?

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 2 Apr 2026 20:08:18 +0200

> asking, what filesystem are those animals running on top of?

btrfs

-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 14:47:52 -0400

> btrfs

Yup, so capable of making sparse WAL files.  I can reproduce the
problem here, and what I see is

> tar --version
tar (GNU tar) 1.34
Copyright (C) 2021 Free Software Foundation, Inc.
...

> tar -?
...
  -H, --format=FORMAT        create archive of the given format

 FORMAT is one of the following:
    gnu                      GNU tar 1.13.x format
    oldgnu                   GNU format as per tar <= 1.12
    pax                      POSIX 1003.1-2001 (pax) format
    posix                    same as pax
    ustar                    POSIX 1003.1-1988 (ustar) format
    v7                       old V7 tar format
...
*This* tar defaults to:
--format=posix -f- -b20 --quoting-style=escape --rmt-command=/usr/bin/rmt
--rsh-command=/usr/bin/ssh

So there you have it: pax format by default.  This is unlike what
I see on RHEL or Fedora:

...
*This* tar defaults to:
--format=gnu -f- -b20 --quoting-style=escape --rmt-command=/etc/rmt
--rsh-command=/usr/bin/ssh

So it looks like we need a switch hack similar to what we did for
BSD tar, but injecting "--format=gnu" (or perhaps "--format=ustar"?)
if the tar program will take that.

Interestingly, pg_verifybackup's t/003_corruption.pl test also fails
with the same issue, so apparently this platform is even more
aggressive about sparse-ifying files than Thomas' FreeBSD box.
I wonder how come we managed to pass that test case before on
these machines.

I'm inclined to push the logic for selecting these tar options
into some common subroutine in Test::Utils, rather than having
two copies (and maybe more later).

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 15:15:36 -0400

> I wonder how come we managed to pass that test case before on

The answer to that seems to be that the test scripts for
pg_verifybackup simply fail to detect when it's mishandling
sparse tar entries.  We only check failing cases not successful
cases, and in each case the error checked for is independent of
whether we would have extracted WAL data correctly.  Grumble.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 11:41:33 +1300

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 18:50:50 -0400

> Looks like sparse files and BTRFS might be a red herring then, if it's
> simply been told to put a pax header on every file?

It looked to me that it was only putting a pax header on sparse-ified
WAL files.

> How about using --format=ustar, instead of that sparse control stuff?

I did it that way for GNU tar, but did not research whether bsdtar
will take that option.  Feel free to hack on ebba64c08 some more.

(It seems though that the two tars' locutions for "write to stdout"
are different, so we might have to have separate tests even if they
end up pushing the same option.)

			regards, tom lane

---

From: Sami Imseih <[email-masked]>
Date: Thu, 2 Apr 2026 18:43:33 -0500

Hi,

I just encountered a regression test failure for pg_waldump due to ebba64c08d9.

````
――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀
――――――――――――――――――――――――――――――――――――――――――――――――――――――
Listing only the last 100 lines from a long log.
#   at /local/home/simseih/pgdev/installations/worktrees/dev/src/bin/pg_waldump/t/001_basic.pl
line 432.
#          got: 'pg_waldump: error: could not find WAL in archive
"pg_wal.tar.gz"
# '
#     expected: ''
```

and regress_log_001_basic shows this:

```
# Running: /usr/bin/tar --format=ustar -cf /tmp/ja26rXZOnb/pg_wal.tar
archive_status 000000010000000000000002 000000010000000000000001
summaries 000000010000000000000003
[22:25:00.525](0.008s) not ok 101
[22:25:00.525](0.000s) #   Failed test at
/local/home/simseih/pgdev/installations/worktrees/dev/src/bin/pg_waldump/t/001_basic.pl
line 350.
[22:25:00.525](0.000s) # ---------- command failed ----------
[22:25:00.526](0.000s) # /usr/bin/tar --format=ustar -cf
/tmp/ja26rXZOnb/pg_wal.tar archive_status 000000010000000000000002
000000010000000000000001 summaries 000000010000000000000003
[22:25:00.526](0.000s) # -------------- stderr --------------
[22:25:00.526](0.000s) # /usr/bin/tar: value 10012663 out of uid_t
range 0..2097151
```

The --format=ustar has a limit of 2^21 (2097151) for UID/GID [1]
and on my machine the UID is 10012663.

So I found that one way to deal with this is to run the tar command with
--owner=0 --group=0. As far as I can tell, the owner and group IDs don't
matter for these tests, so maybe that is OK.

@@ -1333,6 +1333,10 @@ sub tar_portability_options
                == 0)
        {
                push(@tar_p_flags, "--format=ustar");
+               # ustar format supports UIDs only up to 2^21 (2097151).
+               # Override owner/group to avoid failures on systems where
+               # the running user's UID/GID exceeds that limit.
+               push(@tar_p_flags, "--owner=0", "--group=0");
        }

While this fixes the test, I am now not sure what the broader implications are
for --format=ustar for pg_waldump in the broader discussion?

[1] [https://www.gnu.org/software/tar/manual/html_section/Formats.html]

--
Sami Imseih
Amazon Web Services (AWS)

---

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 12:49:21 +1300

> end up pushing the same option.)

I have:

$ tar --version
bsdtar 3.8.2 - libarchive 3.8.2 zlib/1.3.1 liblzma/5.8.1 libzstd/1.5.2
openssl/3.5.4 libb2/bundled
$ gtar --version
tar (GNU tar) 1.35
Copyright (C) 2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by John Gilmore and Jay Fenlason.

This seems to work for both:

$ tar --format=ustar -c /dev/null  > /dev/null
tar: Removing leading '/' from member names
$ gtar --format=ustar -c /dev/null  > /dev/null
gtar: Removing leading `/' from member names

The attached passes with both, and regress_log_001_basic looks like:

# Running: /usr/bin/tar --format=ustar -cf /tmp/J_ifbfUOSd/pg_wal.tar
archive_status 000000010000000000000001 000000010000000000000003
000000010000000000000002 summaries
[12:12:24.301](0.072s) ok 101

# Running: /usr/local/bin/gtar --format=ustar -cf
/tmp/pbdsHdrAdw/pg_wal.tar 000000010000000000000002 archive_status
000000010000000000000003 summaries 000000010000000000000001
[12:18:14.739](0.050s) ok 101

I think a Windows system could be using either.  BSD tar comes
pre-installed by Microsoft and people often install GNU tools.  So I
think we should use File::Spec->devnull() instead of /dev/null, and
Andrew showed that working.  I doubt Windows is capable of making
sparse files (except perhaps with ReFS?), but it's nice to use the
same code everywhere and future-proof in case GNU carries out its
thread to switch to pax by default.  Windows probably has file
attributes that ustar can't represent (?), so I guess that might
motivate it to use pax headers if they are indeed added only when
needed.

Longer term I think we need to tolerate but ignore pax headers.  If I
understand the spirit of this long evolution, pax archives are
intended to be acceptable to pre-pax implementations, which implies
that they can't really change the meaning of the bits of the file
contents.  That's why GNU's --sparse hides funky file encodings from
old tars by renaming them to GNUSparseFile.%p/%f, and that leads back
to my original suggestion that we should figure out how to detect and
reject pax only if we failed to find the file under the expected name.
(Or of course we could just implement support for that, and I have a
half-baked trial patch for that but now is not the time.)
From 0635c0106946ce76c1bb84a5c17b71d0b0e574f7 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 12:03:56 +1300
Subject: [PATCH] Harmonize tar option tests from ebba64c0.

*  GNU and BSD tar both understand --format=ustar.
*  Windows lacks /dev/null, but perl knows its local name.

Discussion: https://postgr.es/m/3676229.1775170250%40sss.pgh.pa.us
Backpatch-through: 18
---
 src/test/perl/PostgreSQL/Test/Utils.pm | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 120999f6ac9..05e1698efa6 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1328,21 +1328,14 @@ sub tar_portability_options
 
 	# GNU tar typically produces gnu-format archives, which we can read fine.
 	# But some platforms configure it to default to posix/pax format, and
-	# apparently they enable --sparse too.  Override that.
-	if (system("$tar --format=ustar -c -O /dev/null >/dev/null 2>/dev/null")
+	# apparently they enable --sparse too.  BSD tar does something similar.
+	# Override that.
+	my $devnull = File::Spec->devnull();
+	if (system("$tar --format=ustar -c $devnull >$devnull 2>$devnull")
 		== 0)
 	{
 		push(@tar_p_flags, "--format=ustar");
 	}
-
-	# bsdtar also archives sparse files by default, but it spells the switch
-	# to disable that differently.
-	if (system("$tar --no-read-sparse -c - /dev/null >/dev/null 2>/dev/null")
-		== 0)
-	{
-		push(@tar_p_flags, "--no-read-sparse");
-	}
-
 	return @tar_p_flags;
 }
 
-- 
2.53.0

---

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 13:07:40 +1300

> +               push(@tar_p_flags, "--owner=0", "--group=0");

Interesting.  BSD tar accepts those too, so here's an update to my
previous patch.

> for --format=ustar for pg_waldump in the broader discussion?

I think users who have their own tar scripts will mostly be
unaffected, but a small minority will see the new error if they try to
use pg_verifybackup or pg_waldump, and they'll find their way to
--format=ustar, and then they might see the UID/GID error in their own
.tar production scripts, and find their way to adding those switches
too.  Seems OK?  Especially with a documentation note once we've
settle all of this.
From a2c065fd5cee53bcde2ccaf92939737e8fc07f06 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 12:03:56 +1300
Subject: [PATCH v2] Improve tar portability logic from ebba64c0.

* GNU and BSD tar both understand --format=ustar.
* Windows lacks /dev/null, but perl knows its local name.
* ustar format doesn't like large UID/GID values, so set them to 0.

Backpatch-through: 18
Co-authored-by: Thomas Munro <[email-masked]>
Co-authored-by: Sami Imseih <[email-masked]>
Discussion: https://postgr.es/m/3676229.1775170250%40sss.pgh.pa.us
Discussion: https://postgr.es/m/CAA5RZ0tt89MgNi4-0F4onH%2B-TFSsysFjMM-tBc6aXbuQv5xBXw%40mail.gmail.com
---
 src/test/perl/PostgreSQL/Test/Utils.pm | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 120999f6ac9..370acfcef7e 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1328,21 +1328,17 @@ sub tar_portability_options
 
 	# GNU tar typically produces gnu-format archives, which we can read fine.
 	# But some platforms configure it to default to posix/pax format, and
-	# apparently they enable --sparse too.  Override that.
-	if (system("$tar --format=ustar -c -O /dev/null >/dev/null 2>/dev/null")
-		== 0)
-	{
-		push(@tar_p_flags, "--format=ustar");
-	}
-
-	# bsdtar also archives sparse files by default, but it spells the switch
-	# to disable that differently.
-	if (system("$tar --no-read-sparse -c - /dev/null >/dev/null 2>/dev/null")
+	# apparently they enable --sparse too.  BSD tar does something similar.
+	#
+	# ustar format supports UIDs only up to 2^21 (2097151).  Override
+	# owner/group to avoid failures on systems where the running user's UID/GID
+	# exceeds that limit.
+	my $devnull = File::Spec->devnull();
+	if (system("$tar --format=ustar --owner=0 --group=0 -c $devnull >$devnull 2>$devnull")
 		== 0)
 	{
-		push(@tar_p_flags, "--no-read-sparse");
+		push(@tar_p_flags, "--format=ustar", "--owner=0", "--group=0");
 	}
-
 	return @tar_p_flags;
 }
 
-- 
2.53.0

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 20:11:37 -0400

> gtar: Removing leading `/' from member names

Cool.  LGTM.

> Andrew showed that working.

Agreed.

> contents.

I don't buy that.  For example, POSIX specifies these allowed
fields in an extended header:

    linkpath
        The pathname of a link being created to another file, of any
        type, previously archived. This record shall override the
        linkname field in the following ustar header block(s).

    path
        The pathname of the following file(s). This record shall
        override the name and prefix fields in the following header
        block(s).

    size
        The size of the file in octets, expressed as a decimal number
        using digits from the ISO/IEC 646:1991 standard. This record
        shall override the size field in the following header
        block(s).

GNU tar seems to try hard to ensure that a non-pax-aware tar can
extract *something* from a tar file, but it's not guaranteed that the
something contains the right data or is located at the right pathname.
It looks like the goal is to allow post-processing to pick up the
pieces.

In any case, this is all completely moot if we don't write code to
de-sparse a sparse entry: we will not be able to validate WAL data
if the WAL file is missing some pages.  So I see little point in
having code that tolerates pax headers if it doesn't also do that.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 13:47:28 +1300

> having code that tolerates pax headers if it doesn't also do that.

Yeah.  FWIW I spent a few hours hacking on that the other day and
could decode many files, but I now realise that the task was made more
difficult by a problem you fixed: without header validation, small
mistakes resulted in corruption or went bananas.  With that now
addressed, I hope I can get it into shape and propose it for the next
cycle...

For what it's worth, I was just speculating about how one might
reasonably handle unrecognised *non-standard* header names, not the
POSIX-standardised ones which, you're right, we'd probably need to
grok properly.  If we assumed reasonable engineering decisions
following (what I understood to be) the spirit of pax, maybe we could
assume that new non-standard headers either don't affect file contents
and thus could be ignored (think: GNU.windows.permissions=...), or do
affect file contents but have measures in place to prevent unknown
encodings from being exposed to unsuspecting software (think:
deathstation.byte=9bit).  That's a position we could choose to take,
anyway, in the absence of a crystal ball...  Fortunately there aren't
really many implementations of POSIX left, so it's not like we're
dealing with the Fermi Paradox here...

---

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 13:59:16 +1300

> Cool.  LGTM.

Thanks.  I'll go ahead and push v2 shortly, which includes Sami's
change (with credit), unless you have any better ideas for that bit.

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 21:11:22 -0400

> change (with credit), unless you have any better ideas for that bit.

Not really.  I wondered for a bit if it was smart to create tar files
whose contents would appear root-owned, but it could only matter if
somebody extracted such a file as root, and I don't see a plausible
pathway for that to happen, since these are merely transient test
files.

We could dodge that hazard by using, say, "--owner=1" but I'm
not sure whether that'd introduce its own problems.  In any case,
walmethods.c's tar_open_for_write is already filling in zeroes
there.

			regards, tom lane

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 3 Apr 2026 14:37:47 +0300

Hi,

> So there you have it: pax format by default.  This is unlike what

It seems that the problem also applies to OpenBSD [1]:

-F format
    Specify the output archive format, with the default format being
pax. tar currently supports the following formats:

OpenBSD CI tasks started to fail [2] after bc30c704ad with the errors:

```
Listing only the last 100 lines from a long log.
#   at /home/postgres/postgres/src/bin/pg_waldump/t/001_basic.pl line 440.
#          got: 'pg_waldump: error: pax extensions to tar format are
not supported

#   Failed test 'corrupt backup fails verification: extra_file: matches'
#   at /home/postgres/postgres/src/bin/pg_verifybackup/t/003_corruption.pl
line 198.
#                   'pg_verifybackup: error: pax extensions to tar
format are not supported

Summary of Failures:
239/381 postgresql:pg_waldump / pg_waldump/001_basic
                 ERROR            18.58s   exit status 84
225/381 postgresql:pg_verifybackup / pg_verifybackup/003_corruption
                 ERROR            45.12s   exit status 8
```

I also tried Thomas'
"v2-0001-Improve-tar-portability-logic-from-ebba64c0" [3] but it
didn't fix the problem on OpenBSD [4].

[1] https://man.openbsd.org/tar#F
[2] https://cirrus-ci.com/task/5439721360326656
[3] https://postgr.es/m/CA%2BhUKGLMkv_fnGXzVRO8qbx5uHs-qMn151GTJYCfn9w1ZamGNg%40mail.gmail.com
[4] https://cirrus-ci.com/task/5602126958690304

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Thomas Munro <[email-masked]>
Date: Sat, 4 Apr 2026 03:37:26 +1300

> didn't fix the problem on OpenBSD [4].

Apparently it wants -F ustar, like this.  Funny that it passed on the
build farm animals though.  Oh, it looks like they changed the default
fairly recently.

https://undeadly.org/cgi?action=article;sid=20240417053301
From 4b69f7d64798a5a55fdbb2447cee7a0d4326280c Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 3 Apr 2026 12:03:56 +1300
Subject: [PATCH v3] Improve tar portability logic from ebba64c0.

* GNU and BSD tar both understand --format=ustar.
* Windows lacks /dev/null, but perl knows its local name.
* ustar format doesn't like large UID/GID values, so set them to 0.
* OpenBSD has its own tar which understands -F ustar.

Backpatch-through: 18
Co-authored-by: Thomas Munro <[email-masked]>
Co-authored-by: Sami Imseih <[email-masked]>
Reviewed-by: Tom Lane <[email-masked]>
Reviewed-by: Nazir Bilal Yavuz <[email-masked]>
Discussion: https://postgr.es/m/3676229.1775170250%40sss.pgh.pa.us
Discussion: https://postgr.es/m/CAA5RZ0tt89MgNi4-0F4onH%2B-TFSsysFjMM-tBc6aXbuQv5xBXw%40mail.gmail.com
---
 src/test/perl/PostgreSQL/Test/Utils.pm | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 120999f6ac9..077305cd790 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1328,21 +1328,24 @@ sub tar_portability_options
 
+	# apparently they enable --sparse too.  BSD tar (libarchive) does something
+	# similar.
 
-	# bsdtar also archives sparse files by default, but it spells the switch
-	# to disable that differently.
-	if (system("$tar --no-read-sparse -c - /dev/null >/dev/null 2>/dev/null")
-		== 0)
+	# OpenBSD's tar also defaults to pax, but spells the switch differently.
+	if (system("$tar -F ustar -c $devnull >$devnull 2>$devnull"))
 	{
-		push(@tar_p_flags, "--no-read-sparse");
+		push(@tar_p_flags, "-F", "ustar");
 	}
-
 	return @tar_p_flags;
 }
 
-- 
2.47.3

---

From: Sami Imseih <[email-masked]>
Date: Fri, 3 Apr 2026 09:59:19 -0500

Hi,

> build farm animals though.  Oh, it looks like they changed the default

LGTM with just a correction of my earlier comment.

< +    # ustar format supports UIDs only up to 2^21 (2097151).  Override
---
> +    # ustar format supports UIDs only up to 2^21 - 1 (2097151).  Override

--
Sami

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Fri, 3 Apr 2026 18:30:01 +0300

Hi,

> LGTM with just a correction of my earlier comment.

Thanks for the patches! I confirm that both v3 and v4 fix the problem
for OpenBSD CI.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Thomas Munro <[email-masked]>
Date: Sat, 4 Apr 2026 14:07:04 +1300

> Thanks for the patches! I confirm that both v3 and v4 fix the problem

Pushed, after testing on an OpenBSD VM and making some corrections:

* I'd screwed up the test command line in a way that worked by coincidence
** OpenBSD tar writes to a tape device by default, so use -f /dev/null
** I'd forgotten == 0, so the result was inverted, hiding that screwup
* -f /dev/null is a better form for all of them because the default
destination is a build option
* needed elsif instead of if, or BSD tar finished up getting both
--format=ustar and -F ustar
* ran perltidy, keeping only the hunks due to this patch

CI passes and shows "212 subtests passed" for all five Unixen +
Windows/mingw, but only "156 subtests passed" for Windows/MSVC.
.cirrus.tasks.yml appears to use the same $TAR for both, namely the
system tar, so I think we can say that *this* thing is working, but
something else might be wrong with our scripting glue somewhere?

The other OSes on our list are AIX and Solaris.  From a quick look at
their manuals, I don't foresee issues with pax or large UIDs.

Hopefully that covers everything!

---

From: Thomas Munro <[email-masked]>
Date: Fri, 10 Apr 2026 19:57:56 +1200

Nitpicking code review for commit b15c1513:

+read_archive_wal_page(XLogDumpPrivate *privateInfo, XLogRecPtr targetPagePtr,
+                     Size count, char *readBuff)

I thought we agreed to stop using Size for new code?  size_t has been
around since C89.

+               pg_fatal("WAL segment \"%s\" in archive \"%s\" is too short: rea
d %lld of %lld bytes",
+                        fname, privateInfo->archive_name,
+                        (long long int) (count - nbytes),
+                        (long long int) count);

Why cast to long long int?  That's the sort of thing we used to have
to do for int64 (but no longer), but here it's size_t anyway.  %zu has
been around since C99.

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 10 Apr 2026 08:57:14 -0400

> I thought we agreed to stop using Size for new code?  size_t has been

Must have missed the memo :-(

> to do for int64 (but no longer), but here it's size_t anyway.  %zu has

will fix. Thanks for looking.

cheers

andrew