Horton
Menu

Re: pg_waldump: support decoding of WAL inside tarfile

Tom Lane <tgl@sss.pgh.pa.us>

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Michael Paquier <michael@paquier.xyz>, Amul Sul <sulamul@gmail.com>, Zsolt Parragi <zsolt.parragi@percona.com>, Robert Haas <robertmhaas@gmail.com>, Chao Li <li.evan.chao@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-21T22:24:07Z
Lists: pgsql-hackers 
I wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I also noticed a possible bug in astreamer, where the decompressor 
>> finalize functions send bbs_buffer.maxlen bytes to the next streamer 
>> when flushing remaining data at end-of-stream. This seems wrong because 
>> the buffer may only be partially filled with valid decompressed data. 
>> Possible patch for that attached. (But I don't think it's related to 
>> these failures).

> Ugh.  That's surely very broken, but how did we not notice?
> Do all the consumers know enough to ignore garbage trailing data?

I poked into this question and found that all of our existing uses
of the decompression astreamers are in front of astreamer_tar.c's
astreamer_tar_parser_ops.  Since a tar file has a trailer (two
zero blocks), it is not surprising that garbage after the trailer
will be ignored.  It might appear that astreamer_tar_parser_content
itself will complain about such extra data:

            case ASTREAMER_ARCHIVE_TRAILER:

                /*
                 * 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.
                 */
                astreamer_buffer_bytes(streamer, &data, &len, len);
                if (len > 2 * TAR_BLOCK_SIZE)
                    pg_fatal("tar file trailer exceeds 2 blocks");
                return;

But this code is itself buggy!  The astreamer_buffer_bytes call
is guaranteed to reduce "len" to zero, therefore that pg_fatal
call can never fire, no matter how much garbage we just stuffed
into the astreamer's buffer.

We might want to do

-               if (len > 2 * TAR_BLOCK_SIZE)
+               if (streamer->bbs_buffer.len > 2 * TAR_BLOCK_SIZE)

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.

However, I'm a bit loath to make that change even after fixing the
decompressors, because this discovery suggests that we may currently
be building tar files that contain garbage after the trailer.
And it seems to me that such tar files are perfectly legal anyway.
The POSIX spec for "ustar" format (found under pax(1)) says

    At the end of the archive file there shall be two 512-octet
    logical records filled with binary zeros, interpreted as an
    end-of-archive indicator.

    The logical records may be grouped for physical I/O operations, as
    described under the -bblocksize and -x ustar options. Each group
    of logical records may be written with a single operation
    equivalent to the write() function. On magnetic tape, the result
    of this write shall be a single tape physical block. The last
    physical block shall always be the full size, so logical records
    after the two zero logical records may contain undefined data.

That last sentence seems to say that you can put arbitrary garbage
after the trailer without invalidating the file.  Sure, it's a
holdover from magtape days, but that's how the spec is written.

Therefore, I think we might be best off to just remove this
nonfunctional error check, rather than "fix" it.  Another
idea which might be a bit better is to drop any data after
the standard trailer on the floor, instead of passing it on
as the code now does.

Thoughts?

			regards, tom lane

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use size_t instead of Size in pg_waldump

  2. More tar portability adjustments.

  3. Further harden tests that might use not-so-compatible tar versions.

  4. Harden astreamer tar parsing logic against archives it can't handle.

  5. Fix pg_waldump/t/001_basic.pl with BSD tar on ZFS.

  6. Remove a low-value, high-risk optimization in pg_waldump.

  7. Fix misuse of simplehash.h hash operations in pg_waldump.

  8. Fix file descriptor leakages in pg_waldump.

  9. Fix poorly-sized buffers in astreamer compression modules.

  10. Remove read_archive_file()'s "count" parameter.

  11. Report detailed errors from XLogFindNextRecord() failures.

  12. Fix assorted bugs in archive_waldump.c.

  13. Remove nonfunctional tar file trailer size check.

  14. Fix finalization of decompressor astreamers.

  15. Move tar detection and compression logic to common.

  16. pg_verifybackup: Enable WAL parsing for tar-format backups

  17. pg_waldump: Add support for reading WAL from tar archives

  18. pg_waldump: Preparatory refactoring for tar archive WAL decoding.

  19. pg_waldump: Remove file-level global WalSegSz.

  20. pg_verifybackup: Verify tar-format backups.