Re: pg_waldump: support decoding of WAL inside tarfile - Mailing list pgsql-hackers

On Wed, Jan 28, 2026 at 8:32 AM Euler Taveira <euler@eulerto.com> wrote:
>
> 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.

> > I have included your patch (removing WalSegSz) to the attached patch
> > set as 0001; all my subsequent patches are now bumped by one.
> >
> 0003. If this is a refactor, you should move the unlikely logic (if necessary)
> to 0005.
>

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.

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

Okay, will do it in the next updated version.

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

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

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

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


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

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

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

Robert responded to this -- thanks, Robert.

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

Okay.

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

Okay.

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

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
> common file (common/compression.c?) and use it in other places like
> CreateBackupStreamer() -- pg_basebackup.
>

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

> +   /* :. */
> +   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'.)
>

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

> +   /* Hash table storing WAL entries read from the archive */
> +   ArchivedWAL_HTAB = ArchivedWAL_create(16, NULL);
>
> Any reason for 16? Comment says nothing about it.
>

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

> +       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\".
>

Okay.

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

Okay.

> -  dependencies: [frontend_code, lz4, zstd],
> +  dependencies: [frontend_code, lz4, zstd, libpq],
>
> Put libpq after frontend_code.
>

Okay.

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

Okay.

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

Okay.

> +/* Forward declaration */
> +struct ArchivedWALFile;
>
> Why don't you move ArchivedWALFile to pg_waldump.h? pg_waldump.h is included
> into archive_waldump.c.
>

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.

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

Okay.

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

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

>   could not change permissions of \"%s\": %m
>
> +   pg_log_debug("temporarily exporting file \"%s\"", fpath);
>
> spilling to temporary file \"%s\"
>

Okay.

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

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