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

From: Amul Sul <[email-masked]>
Date: Fri, 2 Jan 2026 17:59:54 +0530

Attached is the rebased version, includes some code comment improvements.

Regards,
Amul

---

From: Robert Haas <[email-masked]>
Date: Fri, 16 Jan 2026 15:38:03 -0500

> Attached is the rebased version, includes some code comment improvements.

Regarding 0001, I don't think that passing the WAL segment size around
in a global variable is a good practice. It makes it hard to tell
whether the variable is guaranteed to have been set properly at all
the places where it is used. Of course, this patch set does not
introduce that problem, but I think it could avoid making it worse by
adding WalSegSz to XLogDumpPrivate instead of exporting the global
variable everywhere. That struct seems to be available everyplace
where we need the value. Actually, though, I'm inclined to remove the
file-level global as well. Patch for that attached. I'm not against
global variables in general, but I'm against global variables that are
just there to avoid passing the right stuff around via function
argument lists.

0002 claims to be just a refactoring but if
(unlikely(private->endptr_reached)) return -1 is net new code, so I
object. I realize this contradicts the review comment from Chao Li,
but the principal that refactoring shouldn't change the logic is more
important than whatever value we might get out of adding this sanity
check here -- and I bet if we look into it we'll find that there are
lots of other places in PostgreSQL that would also need to be changed
if we wanted such a sanity check everywhere that we have code like
this. I'd also like to point out that the commit message isn't really
adequate to understand why this helps achieve the overall goal of the
patch set.

0003 looks functionally correct but notice that you've stolen some but
not all of the tests from the "# various ways of specifying WAL range"
section without copying the comment or explaining in the commit
message why you've moved some tests but not others.

Regarding 0004:

Instead of making ArchivedWAL_HTAB a global variable, can we just
store a pointer to it in XLogDumpPrivate? (I would change the name to
something else, something all lower case.) Or maybe the pointer should
go someplace else, but I'm skeptical of a global variable here for the
same reasons as 0001.

I think ArchivedWALFile would be a better name than ArchivedWALEntry,
and cur_wal_file or cur_file would be a better name than cur_wal.

I'm extremely happy with the way that the hash table looks and the new
logic in init_archive_reader() to make sure that we don't need to
reread the beginning of the WAL but can reuse the data already read.
That seems like a huge improvement to me.

+               if (recptr >= endPtr)
+               {
+                       /* Discard the buffered data */
+                       resetStringInfo(&entry->buf);
+                       len = 0;
+
+                       /*
+                        * Push back the partial page data for the
current page to the
+                        * buffer, ensuring it remains full page
available for re-reading
+                        * if requested.
+                        */
+                       if (p > readBuff)
+                       {
+                               Assert((count - nbytes) > 0);
+                               appendBinaryStringInfo(&entry->buf,
readBuff, count - nbytes);
+                       }
+               }

It feels pretty inelegant to me to pull data back into &entry->buf
from readBuff here. I think what you should do is just do this test
once, before entering the while (nbytes > 0) loop. Then you'll always
have p == readBuff, so the nested if statement is no longer required.
Also, the way you have it, if this triggers for the first loop
iteration, it will trigger for every subsquent loop iteration, which
seems wasteful.

+               if (len > 0 && recptr > startPtr)
+               {
+                       int                     skipBytes = 0;
+
+                       /*
+                        * The required offset is not at the start of
the buffer, so skip
+                        * bytes until reaching the desired offset of
the target page.
+                        */
+                       skipBytes = recptr - startPtr;
+
+                       buf += skipBytes;
+                       len -= skipBytes;
+               }

This logic seems pretty confusing. It looks as though len can go
negative, because what if skipBytes > len? I'm not sure that's really
possible here, but there's no comments explaining why it can't and no
assertion verifying that it doesn't. I think you should probably try
to unify this with the following if statement that is gated by (len >
0). In other places where we have code like this, what I've usually
found useful is to compute the number of bytes to copy as Min(bytes
available from source, bytes that will fit into the target), and I
think that technique might be useful here. For example, in
BlockRefTableRead, length is always the number of bytes that the
caller wants which we haven't yet read, and we compute bytes_to_copy =
Min(length, buffer->used - buffer->cursor). We then memcpy(data,
&buffer->data[buffer->cursor], bytes_to_copy) and then adjust data and
length. A similar bit of code not written by me can be found in
WALReadFromBuffers, which maintains nbytes as the remaining number of
bytes to be copied and then computes the size of the next copy as
npagebytes = Min(nbytes, XLOG_BLCKSZ - offset). I think your if (len >
0 && recptr > startPtr) block is trying to do something similar to
what the Min() is doing in those examples, but I find it hard to
understand whether it is doing it correctly. I think you need some
combination of clearer variable names, more comments, and/or rewriting
this to be easier to read and understand without help.

+               entry = privateInfo->cur_wal;
+
+               /* Fetch more data */
+               if (read_archive_file(privateInfo, READ_CHUNK_SIZE) == 0)
+                       break;                  /* archive file ended */

Why do we set entry to privateInfo->cur_wal and then call
read_archive_file() afterwards? Doesn't read_archive_file() have the
potential to change cur_wal? If so, don't we want to look at the entry
that is current after we've called read_archive_file(), rather than
before? Also, to harp on the naming a bit more, stuff like entry =
privateInfo->cur_wal kind of shows that you haven't made the naming
real consistent. This could be more clear if it said something like
wal_file = privateInfo->cur_wal_file.

+               /* Found the required entry */
+               if (entry->segno == segno)
+                       return entry;
+
+               /*
+                * Ignore if the timeline is different or the current
segment is not
+                * the desired one.
+                */
+               if (privateInfo->timeline != entry->timeline ||
+                       privateInfo->startSegNo > entry->segno ||
+                       privateInfo->endSegNo < entry->segno)
+               {
+                       privateInfo->cur_wal = NULL;
+                       continue;
+               }
+

I don't really understand what's happening here. One thing that's odd
is that the first of these two if statements considers the timeline
and the second does not. It seems unlikely that this is correct. I
suspect that the hash table needs to be keyed by (TLI, segno) rather
than just by TLI, and that get_archive_wal_entry needs to take both a
TLI and a segment number as an argument.  Otherwise, what could
possibly prevent the first if statement from returning the correct
segment from the wrong timeline?

(Likewise, I think stuff in 0005 like prepare_tmp_write() should work
on a TLI and a segno, not just a segno. There probably should be very
few places in the patch where you pass only a segno and no TLI.
get_tmp_walseg_path() is a pretty clear sign of the problem: If you
were including the TLI in the file name, you would just use
XLogFileName() here, but because you left out the TLI, you end up
having to invent your own file naming convention. That seems pretty
bad: wouldn't we like the temporary file names to have the same names
as the files inside the archive? What happens if the same WAL file
exists in the archive with two different TLIs?)

Another thing that's odd is: why is this function resetting
privateInfo->cur_wal to NULL? I feel like it should be the job of
astreamer_waldump_content() to manage the value of
privateInfo->cur_wal, and this function doesn't seem to have any
business touching it. Even if it thinks that we want to ignore that
WAL file for purposes of *this* call to get_archive_wal_entry(),
surely it has no right to decide that the *next* call to this function
should ignore that entry as well.

Honestly, I don't really understand why we need to poke into
privateInfo at all here. I feel like the shape of this loop should be:
while (1) { if (read_archive_file(...) == 0) break; if (we have the
correct entry) return entry; } }. It seems to me that it ought to be
the job of the archive streamer to handle everything else, including
buffering more data and spilling to files. This should just iterate,
repeatedly calling read_archive_file(), until we either get to the
file we want or run out of archive.

+               /*
+                * 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;

I couldn't figure out what was going on here for a while. Then I
realized that this makes some sense: if (segno > entry->segno), that
means that the segment number that we've found in the archive is older
than the requested segment number. Because we don't back up, that
means we don't need the data. I think the idea is that we would fall
through here and buffer the data if this if-test fails, but patch 0005
actually removes these two lines of code, so I think something here is
kind of messed up. I think the result will be that with both patches
applied, we'll buffer data we don't actually need for anything.

Generally, I think that some details here should be revisited:

+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;

The first few members are great, except that timeline needs to be part
of the hash key as well, so you probably need to make a two-element
struct with a segno and a TLI and make that struct be the thing that
comes right after status. After that, I'm not so sure. Essentially, I
think what's happening here is that segno*file_size+total_read is the
end LSN for buf, and the start LSN value is that value minus buf.len.
And if tmpseg_exists is true, then the buffer might be empty, and you
can read however much of the segment we found from the temp file. I
find that confusing. If we're going to do something like this, I feel
like it would make more sense to store XLogRecPtr starttli instead of
int total_read, so that we explicitly mention the first LSN stored in
the buffer, and you infer the ending LSN from the length of the
buffer.

But, stepping back a bit, why do we have a StringInfoData in every
ArchivedWALEntry? It makes sense to do it that way if we're going to
buffer all of the WAL data in memory. In that case, you need a
separate buffer for every ArchivedWALEntry, and so this is logical.
But if we're going to spill to disk, then there should only ever be
one buffer in use at any given time, so why have a buffer in every
ArchivedWALEntry instead of a single buffer inside of XLogDumpPrivate?
We could choose to treat the hash table as the set of previous
segments for which we have written data to disk, and the current
segment and any associated buffer would be tracked inside the
XLogDumpPrivate or astreamer_waldump, which would mean that only one
copy of them would exist. I'm tempted to say that the direction I'm
sketching here is the right way to go, but I'm not 100% certain that's
true. It could be write to have a buffer per WAL file, as you have
here, but if so, we should know why we're doing that and how it fits
into the design, and I'm not currently seeing a real reason for it.

+       if (strstr(member->pathname, "PaxHeaders."))
+               return false;

There is no way that a filename containing the string "PaxHeaders."
could ever pass the IsXLogFileName test just above. We shouldn't need
this.

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

---

From: Amul Sul <[email-masked]>
Date: Mon, 19 Jan 2026 16:43:30 +0530

> > Attached is the rebased version, includes some code comment improvements.

Thanks for the feedback. I am replying to a few comments inline below
regarding elements that are unlikely to change -- explaining the
reasoning why I believe they are correct -- just to ensure we are
aligned. For the remaining points, I generally agree and will either
implement the improvements or provide my reasoning if I don't.

> 0).

The preceding check if (recptr >= endPtr) ensures that within this
block, recptr is always less than endPtr. Since startPtr is derived as
endPtr - len, recptr naturally falls within the expected range (i.e.,
recptr - startPtr <= len). However, I agree that we can simplify this
by combining it with the subsequent if statement and/or by adding an
assert there.

> wal_file = privateInfo->cur_wal_file.

I see your point, and you’re right that assigning the entry to
privateInfo->cur_wal here seems useless; however, it becomes necessary
for the logic in patch 0005. I assigned it just before
read_archive_file() because that function can change where the current
WAL ends and a new one starts. I need to track the completed WAL entry
at that moment, as it may contain data that needs to be spilled to a
temporary file. I'll add a comment to clarify this. I also agree with
your feedback on variable naming and will improve that in the next
revision.

> segment from the wrong timeline?

Yeah, I need to swap the positions of these checks.

> file we want or run out of archive.

We previously tried having the astreamer function perform all
necessary checks (e.g., TLI and segment number), but this became
problematic in init_archive_reader when we needed to read a single WAL
page just to determine the WAL segment size without any validation. To
bypass those checks, I initially used a READ_ANY_WAL() macro, but it
felt like a hack.

In the current design, the archive streamer is responsible only for
reading and copying data to the buffer. To optimize this and avoid
unnecessary memcpy and  data buffering for WAL files that aren't
needed, we set privateInfo->cur_wal to NULL, signaling the streamer
code to skip those operations.

But, I am thinking that instead of setting privateInfo->cur_wal to
NULL, we could simply discard the buffer data at that place and let
memcpy in astreamer_content copy if it would be that much of an issue.

> applied, we'll buffer data we don't actually need for anything.

Yeah, should discard buffered data too.

> the buffer, and you infer the ending LSN from the length of the

total_read is not only the total of buffered data; it also includes
data that has been spilled to a temporary file. If the buffer is not
empty, the most recently read data from the archive exists there;
otherwise, if a temporary file exists, that data will be found there.
In short, it tracks the total bytes of WAL that have been read from
the archive. I will improve the code comment to reflect this.

> into the design, and I'm not currently seeing a real reason for it.

In the earlier version, we had only one buffer where we copied
streamed data, but a problem (previously discussed [1]) arose when the
archive streamer reads data from two WAL files -- i.e., one is ending
and the next is starting. We have to keep track of where the previous
file ends and the next one starts, along with the WAL segment
information that each belongs to.

> could ever pass the IsXLogFileName test just above. We shouldn't need

This checks the directory name (e.g.,
x_dir/y_dir/PaxHeaders.NNNN/wal_file_name). The name of that metadata
file is exactly the same as the WAL file name, which is why
IsXLogFileName() doesn't help here.

Regards,
Amul

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

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Jan 2026 08:30:48 -0500

> code to skip those operations.

Hmm. In that case, the code definitely deserves a comment, as that was
not at all clear to me.

> memcpy in astreamer_content copy if it would be that much of an issue.

I don't think doing unnecessary copying is the right way forward. The
copying itself could be expensive, but I am a little concerned about
the memory utilization of this code. Suppose the user has increased
the WAL segment size to 1GB or even higher. It seems like we could
buffer a whole segment or maybe even more in some scenarios. If we
avoid copying data we don't need, then we also avoid buffering it in
memory.

In terms of the separation of concerns, we could view setting
privateInfo->cur_wal = NULL as a form of signaling, a way for this
code to tell the astreamer that it doesn't need the data buffering.
However, I think it might be better to make the signaling more
explicit. Instead of having the caller directly set the buffer to
NULL, or directly trim data out of the buffer, maybe it should set
some value in privateInfo that tells the astreamer what to do. For
instance, suppose it sets the oldest LSN that it might still care
about in privateInfo, and then the astreamer is free to do skip
copying of any data prior to that LSN, and discard any that it already
has. Especially if properly commented, I think this might be more
clear than what you have now. I am not 100% sure that's the right
idea, though. I just think right now it's a bit murky who does what
and why the division of responsibilities is what it is.

For example, read_archive_wal_page() currently has some responsibility
for discarding data, but is that because it is the right place to
discard data, or is that just because that's where we have an LSN
available that tells us what we can discard? I don't know, but an
explicit signaling mechanism like what I describe in the previous
paragraph can make it possible for those two things to happen in
different places.

> information that each belongs to.

Hmm, interesting. My first thought was that if we were starting a new
WAL file, isn't it time to flush the data from the old one to a spill
file? But maybe it's not, if the WAL reader can reread records, and a
record can span file boundaries. But probably we need some comment so
that the next reader is not confused, and we definitely need to make
sure that the structure members in ArchivedWALEntry are well-chosen
and well-documented.

> IsXLogFileName() doesn't help here.

I think this code should only be considering files in the toplevel
directory, and skipping over any directories it finds. I absolutely
promise I am not going to commit anything that is specifically looking
for PaxHeaders. Nothing we've ever done with tar files up to now has
required that, and I don't think this should, either.

---

From: Amul Sul <[email-masked]>
Date: Fri, 23 Jan 2026 17:57:11 +0530

> and why the division of responsibilities is what it is.

The current implementation of astreamer_waldump_content() does not
have sufficient information to skip WAL segments during the initial
hash entry preparation and data-copying phase. Because the filtration
parameters -- which determine if a segment should be skipped -- depend
on the WAL segment size, we must first read a WAL page through the
streamer to calculate that size, which is done in
init_archive_reader(). Therefore, the responsibility of the archive
streamer is strictly to copy the WAL segment data to the buffer.

The skipping decision is handled inside get_archive_wal_entry(), which
sets privateInfo->cur_wal to NULL. In the next version, I am planning
to add a separate routine (with better commenting) that, along with
setting the pointer to NULL, releases that hash entry to avoid
unnecessary memory usage.

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.

Therefore, I believe performing the filtration check through
get_archive_wal_entry() and then calling a routine to clear
privateInfo->cur_wal and the hash entry is the better approach, IMO.
Additionally, once we consume a WAL file and move to the next one, the
hash entry and buffer for that WAL can be released to prevent
unnecessary memory consumption by calling the same routine that I am
planning to add.

> required that, and I don't think this should, either.

Ok, fair enough, my intention was to allow decoding of valid WAL data
from any directory in the tar archive, but I will go ahead and add
that restriction as suggested.

Regards,
Amul

---

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

> > Attached is the rebased version, includes some code comment improvements.

Here are my replies to the remaining review comments. An updated
version of the patch was posted in the previous email [1].

> just there to avoid passing the right stuff around via function

Yes, it looks good. Added to the v11 patch set.

> adequate to understand why this helps achieve the overall goal of the

Understood, removed it.

> message why you've moved some tests but not others.

The excluded tests provide WAL files as input to pg_waldump, which is
incompatible with tar archive input. I have updated the commit message
to reflect this.

> go someplace else, but I'm skeptical of a global variable here for the

Done.

> and cur_wal_file or cur_file would be a better name than cur_wal.

Done.

> iteration, it will trigger for every subsquent loop iteration, which

I'm not sure I follow the logic. How would this trigger a copy from
readBuff back into &entry->buf on every iteration? I  believe that
this is a rare case: it should only happen when we reach the end of
the buffer with less than the requested 8kB available. In that
scenario, we copy the remaining bytes, reset the buffer, and then --
since (p > readBuff)  -- perform the pull-back before invoking the
archive stream for the next segment. Am I missing something in the
loop condition?

I replied to the rest of the review comments previously, but I’m not
sure whether replying in multiple emails is the correct approach.
Perhaps, since the patch is taking a bit longer, I wanted to keep the
conversation moving.

Regards,
Amul

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

---

From: Robert Haas <[email-masked]>
Date: Tue, 27 Jan 2026 10:55:23 -0500

> Yes, it looks good. Added to the v11 patch set.

I'll respond to the rest of this later, but I went ahead and committed that one.