Subject: Re: pg_waldump: support decoding of WAL inside tarfile

From: Robert Haas <[email-masked]>
Date: Mon, 26 Jan 2026 12:22:33 -0500

On Fri, Jan 23, 2026 at 7:27 AM Amul Sul <[email-masked]> wrote:
> Another option I previously considered was adding the filtration logic
> inside the archive streamer itself. However, since the very first read
> is required to calculate the WAL segment size, the filter check cannot
> be performed immediately. However, we could send a signal to the
> archive streamer via privateInfo (e.g., a read_any_wal or
> skip_wal_check boolean flag) to disable the filtration check until the
> size is calculated. But that approach isn't very elegant; if the first
> WAL page we read belongs to a segment we actually want to skip, we
> would still have to run the filter check and handle the skip/removal
> logic outside of the streamer (i.e., inside init_archive_reader()).
> This would result in performing the same filtration check in two
> different places.

I mean, I don't really buy this logic. If the information added to
privateInfo is "here's the LSN before which you can remove stuff," and
it starts out initialized to 0/0, then the read of the first WAL page
causes no problem at all, because nothing is before 0/0. After it gets
updated to some non-zero value, the next call to
astreamer_waldump_content() can handle discarding any data we don't
need.

IMHO, the best argument for keeping things are is that in some cases,
that decision might result in a bit of delay in discarding old data,
but I don't think that really matters. I think all that we care about
is the peak memory utilization of an operation, and I don't think that
an explicit signaling system should increase that at all.

That said, I'm certainly willing to consider other ideas about how
this can work. However, I feel strongly that the logic needs to be not
only correct, but clear and well-explained. Setting cur_wal to NULL to
make the astreamer skip without adequate comments doesn't meet that
standard. Maybe with some better comments it's all right, but frankly
I'm a bit skeptical. Right now, you're using whether or not cur_wal is
NULL as a signal to skip data or not skip data. How is that better
than passing down the LSN and TLI that you want to read next and
letting the astreamer figure out what to do itself? It's a signaling
mechanism either way, but it seems a lot easier to figure out whether
we always keep the LSN and TLI updated properly than to figure out
whether cur_wal is always NULL at exactly the right times.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Amul Sul <[email-masked]>
Date: Tue, 27 Jan 2026 17:36:34 +0530

> whether cur_wal is always NULL at exactly the right times.

I am not sure how to calculate the LSN of the very first page it reads
at this point, since we don't have the segment size yet. Furthermore,
the previous implementation using the segment number as the hash key
was incorrect, as extracting the segment number and TLI is impossible
without knowing the segment size, IIUC.

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 have included your patch (removing WalSegSz) to the attached patch
set as 0001; all my subsequent patches are now bumped by one.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Tue, 27 Jan 2026 16:11:26 -0500

> 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.

+static bool
+member_is_wal_file(astreamer_waldump *mystreamer, astreamer_member *member,
+    char **fname)
+{
+ int pathlen;
+ char    *filename;
+
+ /* We are only interested in normal files. */
+ if (member->is_directory || member->is_link)
+ return false;
+
+ pathlen = strlen(member->pathname);
+ if (pathlen < XLOG_FNAME_LEN)
+ return false;
+
+ /* WAL files from the top-level or pg_wal directory will be decoded */
+ if (pathlen > XLOG_FNAME_LEN &&
+ strncmp(member->pathname, XLOGDIR, strlen(XLOGDIR)) != 0)
+ return false;
+
+ /* WAL file could be with full path */
+ filename = member->pathname + (pathlen - XLOG_FNAME_LEN);
+ if (!IsXLogFileName(filename))
+ return false;
+
+ *fname = pnstrdup(filename, XLOG_FNAME_LEN);
+
+ return true;
+}

I don't think this code is fully correct. Note that pg_verifybackup's
member_verify_header() prepends "./" and then applies
canonicalize_path(). I think the same considerations apply here.
Consider:

[rhaas pgdata]$ (cd pg_wal; tar -cf ../pg_wal1.tar 00*)
[rhaas pgdata]$ (cd pg_wal; tar -cf ../pg_wal2.tar .)
[rhaas pgdata]$ tar tf pg_wal1.tar
00000001000000000000002C
00000001000000000000002D
00000001000000000000002E
00000001000000000000002F
000000010000000000000030
000000010000000000000031
000000010000000000000032
000000010000000000000033
[rhaas pgdata]$ tar tf pg_wal2.tar
./
./00000001000000000000002C
./00000001000000000000002D
./00000001000000000000002E
./archive_status/
./000000010000000000000031
./000000010000000000000030
./summaries/
./00000001000000000000002F
./000000010000000000000032
./000000010000000000000033

---

From: "Euler Taveira" <[email-masked]>
Date: Wed, 28 Jan 2026 00:01:39 -0300

> set as 0001; all my subsequent patches are now bumped by one.

I started looking at this patch. This is not a complete review.

0002. LGTM.

0003. If this is a refactor, you should move the unlikely logic (if necessary)
to 0005.

0004. You mentioned in the commit message that this patch should be merged into
0005. Reviewing it is a bit hard if you just looked at the patch. However, it
is clear if you use 'git show -w $HASH0004'. I would expect that this patch
contains

+my $tar = $ENV{TAR};
+

...

+       skip "tar command is not available", 3
+         if !defined $tar;

because you add the SKIP. As a refactor I expect it to work independently.

0005.

+           pg_log_error("unnecessary command-line arguments specified with tar archive (first is \"%s\")",
+                        argv[optind]);

Which command-line arguments? This message isn't clear. How about using "option
%s cannot be used together with tar archive".

-   if (waldir != NULL)                                                         
+   if (walpath != NULL)                                                        
    {                                                                           
+       /* validate path points to tar archive */                               
+       if (is_archive_file(walpath, &compression))                             
+       {                                                                       
+           char       *fname = NULL;                                           
+                                                                               
+           split_path(walpath, &waldir, &fname);                               
+                                                                               
+           private.archive_name = fname;                                       
+           is_archive = true;                                                  
+       }                                                                       
        /* validate path points to directory */                                 
-       if (!verify_directory(waldir))                                          
+       else if (!verify_directory(walpath))

Maybe I'm confused about the fact that the function name is is_archive_file()
and there is a variable called is_archive. Couldn't you test
private.archive_name != NULL instead of using is_archive variable?

-       if (!verify_directory(waldir))                                          
+       else if (!verify_directory(walpath))                                    
        {                                                                       
            pg_log_error("could not open directory \"%s\": %m", waldir);        
            goto bad_argument;

This is not a problem of this patch but I just want to point out that it should
be pg_fatal() just like similar code path a few lines below.

+               /* set segno to endsegno for check of --end */                  
+               segno = endsegno;                                               
+           }                                                                   
+                                                                               
+                                                                               
+           if (!XLByteInSeg(private.endptr, segno, WalSegSz) &&                
+               private.endptr != (segno + 1) * WalSegSz)

2 blank lines. Remove one.

+ * archive_waldump.c
+ *     A generic facility for reading WAL data from tar archives via archive
+ *     streamer.

The other tools (pg_basebackup and pg_verifybackup) that also use astreamer API
named this similar file as astreamer_SOMETHING.c. It seems a good idea to
follow the same pattern, no? Maybe astreamer_tar_archive.c or
astreamer_archive.c.

+/* Hash entry structure for holding WAL segment data read from the archive */
+typedef struct ArchivedWALEntry
+{
+   uint32      status;         /* hash status */
+   XLogSegNo   segno;          /* hash key: WAL segment number */
+   TimeLineID  timeline;       /* timeline of this wal file */
+
+   StringInfoData buf;
+   bool        tmpseg_exists;  /* spill file exists? */
+
+   int         total_read;     /* total read of archived WAL segment */
+} ArchivedWALEntry;

s/wal file/WAL file/

What buf is for? It is the only member that doesn't have a description. I think
total_read gives the impression that you've read the file and that's the number
of bytes it got. That's not true; it represents the accumulative length. Maybe
read_len is a good candidate.

+bool
+is_archive_file(const char *fname, pg_compress_algorithm *compression)
+{
+   int         fname_len = strlen(fname);
+   pg_compress_algorithm compress_algo;
+

Why do you need an extra variable here? I think compress_algo is better than
compression. Besides that, it is a good opportunity to move this function to a
common file (common/compression.c?) and use it in other places like
CreateBackupStreamer() -- pg_basebackup.

+   /* Before that we must parse the tar archive. */
+   streamer = astreamer_tar_parser_new(streamer);
+
+   /* Before that we must decompress, if archive is compressed. */
+   if (compression == PG_COMPRESSION_GZIP)

It reads better if you suppress 'Before that we must'. (I think you want to say
'After' instead of 'Before'.)

+   /* Hash table storing WAL entries read from the archive */
+   ArchivedWAL_HTAB = ArchivedWAL_create(16, NULL);

Any reason for 16? Comment says nothing about it.

+       if (read_archive_file(privateInfo, XLOG_BLCKSZ) == 0)
+           pg_fatal("could not find WAL in \"%s\" archive",
+                    privateInfo->archive_name);

Could we have a better message here? The read_archive_file() is already testing
the error cases and you are testing the EOF case. I would use 'archive \"%s\"'
or even 'archive file \"%s\".

+   /* Needed WAL yet to be decoded from archive, do the same */
+   while (1)
+   {
+       entry = privateInfo->cur_wal;

This comment is not clear. Could you rephrase it?

+       /*
+        * XXX: If the segment being read not the requested one, the data must
+        * be buffered, as we currently lack the mechanism to write it to a
+        * temporary file. This is a known limitation that will be fixed in the
+        * next patch, as the buffer could grow up to the full WAL segment
+        * size.
+        */
+       if (segno > entry->segno)
+           continue;
+
+       /* WAL segments must be archived in order */
+       pg_log_error("WAL files are not archived in sequential order");
+       pg_log_error_detail("Expecting segment number " UINT64_FORMAT " but found " UINT64_FORMAT ".",
+                           segno, entry->segno);

Can it enforce a specific order? tar follows an arbitrary order in which the
files is returned by the filesystem. You've been debating a solution to buffer
the WAL contents using memory or spilled files. If it always create the tar in
an alphabetical order, you can reduce the scope of this patch. (Didn't look
what challenges are expected to use a sorted list to generate the tar file.)

-  dependencies: [frontend_code, lz4, zstd],
+  dependencies: [frontend_code, lz4, zstd, libpq],

Put libpq after frontend_code.

+
+   /* Fields required to read WAL from archive */
+   char       *archive_name;   /* Tar archive name */
+   int         archive_fd;     /* File descriptor for the open tar file */
+
+   astreamer  *archive_streamer;
+
+   /* What the archive streamer is currently reading */
+   struct ArchivedWALEntry *cur_wal;
+
+   /*
+    * Although these values can be easily derived from startptr and endptr,
+    * doing so repeatedly for each archived member would be inefficient, as
+    * it would involve recalculating and filtering out irrelevant WAL
+    * segments.
+    */
+   XLogSegNo   startSegNo;
+   XLogSegNo   endSegNo;

It is a matter of style but consistency is good. Per current style snake case
is preferred instead of CamelCase for these last members.

-       @lines = test_pg_waldump('--path' => $path);
+       my @lines;
+       @lines = test_pg_waldump($path);

my @lines = test_pg_waldump($path);

+/* Forward declaration */
+struct ArchivedWALFile;

Why don't you move ArchivedWALFile to pg_waldump.h? pg_waldump.h is included
into archive_waldump.c.

0006. As I said before I would avoid increasing the size of this patch if an
ordered tar file is viable. Didn't look in deep this patch but I have a few
suggestions:

* I don't like tmpfile_exists as a member name. A suggestion is 'spilled'.

+#ifndef WIN32
+   if (chmod(fpath, pg_file_create_mode))
+       pg_fatal("could not set permissions on file \"%s\": %m",
+                fpath);
+#endif

s/on/of/
My suggestion is to use the same sentence in initdb.

  could not change permissions of \"%s\": %m

+   pg_log_debug("temporarily exporting file \"%s\"", fpath);                   

spilling to temporary file \"%s\"

0007. LGTM.

0008. I'm concerned about this patch. It is breaking backward compatibility if
you are using a long option (--wal-directory). Your proposal is a generic word
that represents both cases (file and directory). I agree. However, I wouldn't
remove --wal-directory from the tool. Instead, I would keep it with the same
short option ('w') but add a sentence saying this long option is deprecated and
will be removed in the future or even remove any traces of this long option
from the help and documentation but silently accept the old long option. I
prefer the latter because it is not a required argument so a deprecation
warning is not necessary IMO.

0009. LGTM.

-- 
Euler Taveira
EDB   https://www.enterprisedb.com/

---

From: Robert Haas <[email-masked]>
Date: Wed, 28 Jan 2026 08:03:02 -0500

> follow the same pattern, no? Maybe astreamer_tar_archive.c or

There shouldn't be anything specific to tar files in here, and
astreamer_archive would be meaningless, since the "a" in "astreamer"
stands for archive. What this file is is an archive streamer specific
to pg_waldump, hence the name.

> what challenges are expected to use a sorted list to generate the tar file.)

It's posible to create a tar file in a specific order by specifying
command-line arguments to tar in the order you want the tar file to be
built. But I think the real thing here is that this limitation is
lifted by the following patch. Whether it's worth splitting it apart
into two patches this way is debatable. As I have pointed out in my
previous reviews, the split hasn't been done very cleanly.

---

From: Amul Sul <[email-masked]>
Date: Wed, 4 Feb 2026 18:39:10 +0530

> 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?

> canonicalize_path(). I think the same considerations apply here.

Understood, will fix that.

Regards,
Amul

---

From: Amul Sul <[email-masked]>
Date: Wed, 4 Feb 2026 19:22:41 +0530

> On Tue, Jan 27, 2026, at 9:06 AM, Amul Sul wrote:

Hi Euler,

Thanks for looking into this, and apologies for the delayed response
due to some unavoidable circumstances.

The updated version of the patch could take a bit more time, as I am
trying to fix and improve the code based on the concerns Robert raised
in his previous post.

> 0003. If this is a refactor, you should move the unlikely logic (if necessary)

Yes, that can be done. I have kept them separate for now to make the
review easier. I can merge them once the patch is ready for the
committer stage, unless the committer prefers to keep them as separate
commit.

> because you add the SKIP. As a refactor I expect it to work independently.

Okay, will do it in the next updated version.

> %s cannot be used together with tar archive".

In the error message, we simply print only the first one -- we do have
similar error messages that use "command-line arguments".

> private.archive_name != NULL instead of using is_archive variable?

Yes, can do that. I hope that doesn't confuse anyone.

> 2 blank lines. Remove one.

That's pre-existing; I'm not sure if I should change this.

> follow the same pattern, no? Maybe astreamer_tar_archive.c or

Robert responded to this -- thanks, Robert.

> +   int         total_read;     /* total read of archived WAL segment */

Okay.

> read_len is a good candidate.

Okay.

> compression.

Well, I follow the practice of updating the output variable at the end
to avoid any garbage assignment once all operations are successful. If
there are strong objections to this, I am happy to change it.

> Besides that, it is a good opportunity to move this function to a

Yes, it would be a good idea to make it common, let me think about this.

> +   /* :. */

I meant to say 'Before'. This is the archive streamer stack, so the
latter one is executed before it.

> Any reason for 16? Comment says nothing about it.

It's an arbitrary choice; I will update the comment.

> or even 'archive file \"%s\".

Okay.

> This comment is not clear. Could you rephrase it?

Okay.

> Put libpq after frontend_code.

Okay.

> is preferred instead of CamelCase for these last members.

Okay.

> my @lines = test_pg_waldump($path);

Okay.

> Why don't you move ArchivedWALFile to pg_waldump.h? pg_waldump.h is included

The full structure is needed in archive_waldump.c only, so I added a
forward declaration to declare the pointer inside XLogDumpPrivate. I
believe we follow such practices elsewhere in the codebase as well.

> * I don't like tmpfile_exists as a member name. A suggestion is 'spilled'.

Okay.

> My suggestion is to use the same sentence in initdb.

I think 'on' seems to be much more relevant than 'of,' and we do have
'could not set permissions on' error messages in other places as wel

> spilling to temporary file \"%s\"

Okay.

> warning is not necessary IMO.

Yeah, that was discussed with Robert offline and we believe that it is
better to make it more generalized; since we can now use the same
option to accept both wal-directory and wal-archived. pg_waldump has
much more generic options for the same, such as -- path=PATH.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Sat, 7 Feb 2026 08:04:32 -0500

> much more generic options for the same, such as -- path=PATH.

Of course, the fact that we discussed it doesn't mean that the issue
is completely settled. However, I don't think there would be general
support from other people on the project for the idea of getting rid
of the long option entirely, or even just the documentation for it. We
have long options for almost all short options these days, and I agree
with that as a general practice. Sometimes we have ONLY a long option,
but we very rarely have ONLY a short option, which is good because
sometimes we have tools with too many options for it to be viable to
give everything a short option, and the long options really help to
make things more self-documenting.

What I would consider a more viable option is to not do the rename and
leave this as --wal-directory even though the argument could really be
a directory or a file. That would avoid the backward compatibility
break that is troubling Euler. And, you could argue that a tar file is
enough like a directory that it won't really cause much confusion.
Personally, I favor renaming it. I think the number of people using
pg_waldump is fairly small, and the number of those people who are
using the long form of the option is very small. Hence, I don't
believe think the rename will inconvenience many users, and I think it
will improve clarity for future users. But, Euler (or someone else)
might take the opposite viewpoint.