Re: pg_waldump: support decoding of WAL inside tarfile

On Sat, Jan 17, 2026 at 2:08 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Jan 2, 2026 at 7:30 AM Amul Sul <sulamul@gmail.com> wrote:
> > 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].

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

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

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

Understood, removed it.

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

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.

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

Done.

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

Done.

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

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/CAAJ_b97mWgAkj7CqdRsof4N_J-gxqjKh79b0Aw3JRo0PoZZCJg@mail.gmail.com