Re: pg_waldump: support decoding of WAL inside tarfile

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> 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()

A few nits:

0001: I think we need to rewrite the commit message now that we've
run the unknowns to ground. Perhaps like

Send the correct amount of data to the next astreamer, not the
whole allocated buffer size. This bug escaped detection because
in present uses the next astreamer is always a tar-file parser
which is insensitive to trailing garbage. But that may not
always be true.

Also, I think we ought to back-patch 0001, just in case we have
misunderstood the scope of usage or somebody tries to make use of
back-branch code for a new purpose. This is a bit tedious because
before f80b09bac/3c9056981 the code was somewhere else and used
different names. But the bugs are clearly there, back to v15.

0002, 0003, 0005: LGTM.

0004: Don't like this at all. It fails to respond to the point
I was trying to make: the code will only consider spilling hash
entries that are the cur_file at some read_archive_file boundary.
Also, it seems to be doubling down on the already over-complicated
and probably under-correct goal of spilling entries that are still
in progress of being read. 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.

regards, tom lane