MIME-Version: 1.0
References: <CAAJ_b94bqdWN3h2J-PzzzQ2Npbwct5ZQHggn_QoYGhC2rn-=WQ@mail.gmail.com>
 <CAAJ_b97PQjE4kFD8Qk6UvtLrfPMixw1nxBz0OP5Z2WB2B-uMxQ@mail.gmail.com>
 <CAAJ_b97JA8ehy_UDddrnGwDt9HG5NmJq8ATtmeMqo7YD-=tLyQ@mail.gmail.com>
 <CA+TgmoZjhWDG_AR1i+L1yss-wbuWvxrdRwSdVUUUnVPrJV2CnQ@mail.gmail.com>
 <CAAJ_b94Uh+b41LQG45bZFK+i62EVvv972LiGWWWuR64=-64rTQ@mail.gmail.com>
 <CA+TgmobF5c7ZcZHdEhqwNxGDZzWG2bDtpRaDtoVELWX_VHs_1A@mail.gmail.com>
 <CAAJ_b94gK1np8d1h-2c1YoCccGXr4zspTa-FC7X_bfXZNz=-DA@mail.gmail.com>
 <CA+TgmoayDY5b+bP1vRRN7A3xOP-=+tK13B2C1g-Xm1j4WTrT9Q@mail.gmail.com>
 <CAAJ_b97JAF+Zuoh2FBO79hVwLeaBPwsbXw-fY+313a7LfRQ-Bg@mail.gmail.com>
 <CA+Tgmoardk4VuthHc23vov+AVkhq7eT0mFUs-2ctAnP1uiTaog@mail.gmail.com>
 <CAAJ_b959x5VjmLJFmN78r_QohQuuj=fde11VbbAOHn5TzgEzng@mail.gmail.com>
 <CAAJ_b97_N+0sipFyq80n0jX-nKcjcQEMOSTVg8DsqkHR8dW_Sw@mail.gmail.com>
 <CAKZiRmyDk5KqovS9Ez3iFHd+p-TChSt2QTtWkwJ5Ya-+4gg21g@mail.gmail.com>
 <CAAJ_b956a+e8-HNEpeJ60ByFv7XJRqECPu3B0dozv0ChMRTPbQ@mail.gmail.com>
 <CAAJ_b94f6sUDWiZY90O-t7SWWeSK0nMWT7AsydKkpAL90m0oMQ@mail.gmail.com>
 <731ADE6F-01C5-4996-BAEE-5851DFC3F502@gmail.com> <CAAJ_b94SEcBVJcsp0Y1-YvLqZWBHPQH4FhRzSJfaH_ah_eL_FQ@mail.gmail.com>
 <CAAJ_b97nn9denP2SAjuXyjwbd3is-VnZVSkiRMJ-5YNiKfO9MA@mail.gmail.com>
 <CA+Tgmob_DB9QHDOxnP7a5Y0yJdeGqY8YNi+uK_811y7cN4mxYA@mail.gmail.com>
 <CAAJ_b97VUiP-DbLNe-ddq64J_RiB4ZcPgAjHkJH-0dbzgjR++A@mail.gmail.com>
 <CA+TgmoYMtcZBaqy9r59eDapaDy3WOdepkFFURu9MV-x-kxEbKg@mail.gmail.com>
 <CAAJ_b95FOeW38gw-3BLmpdnTWHFimopTvf=eTObYUbTOC0x8qg@mail.gmail.com>
 <CA+Tgmob=3POOO8st-v-fCjKCKREQ=+gs5_PBQnoFeNBdERfuEg@mail.gmail.com>
 <CAAJ_b94=gtCeUKkGPUmPj_2SwHV+PiXQ8Mx-1RqfYj3pP3OwpA@mail.gmail.com> <3c8e7b02-2152-495a-a0b6-e37cf9286a70@app.fastmail.com>
In-Reply-To: <3c8e7b02-2152-495a-a0b6-e37cf9286a70@app.fastmail.com>
From: Amul Sul <sulamul@gmail.com>
Date: Wed, 4 Feb 2026 19:22:41 +0530
Message-ID: <CAAJ_b95evCRK2DSWFcTF6rpo3ku-qS=rCwUcNyFjY_4muc3suw@mail.gmail.com>
Subject: Re: pg_waldump: support decoding of WAL inside tarfile
To: Euler Taveira <euler@eulerto.com>
Cc: 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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAAJ_b95evCRK2DSWFcTF6rpo3ku-qS%3DrCwUcNyFjY_4muc3suw%40mail.gmail.com>
Precedence: bulk

On Wed, Jan 28, 2026 at 8:32=E2=80=AFAM Euler Taveira <euler@eulerto.com> w=
rote:
>
> 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 neces=
sary)
> 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 merge=
d 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 pat=
ch
> contains
>
> +my $tar =3D $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 wi=
th 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 !=3D NULL)
> +   if (walpath !=3D NULL)
>     {
> +       /* validate path points to tar archive */
> +       if (is_archive_file(walpath, &compression))
> +       {
> +           char       *fname =3D NULL;
> +
> +           split_path(walpath, &waldir, &fname);
> +
> +           private.archive_name =3D fname;
> +           is_archive =3D 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_fi=
le()
> and there is a variable called is_archive. Couldn't you test
> private.archive_name !=3D 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 =3D endsegno;
> +           }
> +
> +
> +           if (!XLByteInSeg(private.endptr, segno, WalSegSz) &&
> +               private.endptr !=3D (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 arc=
hive
> + *     streamer.
>
> The other tools (pg_basebackup and pg_verifybackup) that also use astream=
er 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 archi=
ve */
> +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 =3D strlen(fname);
> +   pg_compress_algorithm compress_algo;
> +
>
> Why do you need an extra variable here? I think compress_algo is better t=
han
> 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 =3D astreamer_tar_parser_new(streamer);
> +
> +   /* Before that we must decompress, if archive is compressed. */
> +   if (compression =3D=3D 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 =3D 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) =3D=3D 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 t=
esting
> 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 =3D 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 endp=
tr,
> +    * 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 =3D test_pg_waldump('--path' =3D> $path);
> +       my @lines;
> +       @lines =3D test_pg_waldump($path);
>
> my @lines =3D test_pg_waldump($path);
>

Okay.

> +/* Forward declaration */
> +struct ArchivedWALFile;
>
> Why don't you move ArchivedWALFile to pg_waldump.h? pg_waldump.h is inclu=
ded
> 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 f=
ew
> 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 compatibili=
ty 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 woul=
dn't
> remove --wal-directory from the tool. Instead, I would keep it with the s=
ame
> short option ('w') but add a sentence saying this long option is deprecat=
ed and
> will be removed in the future or even remove any traces of this long opti=
on
> 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=3DPATH.

Regards,
Amul