Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Wed, 28 Jan 2026 00:01:39 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Amul Sul" <sulamul@gmail.com>, "Robert Haas" <robertmhaas@gmail.com>
Cc: "Chao Li" <li.evan.chao@gmail.com>,
 "Jakub Wartak" <jakub.wartak@enterprisedb.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Message-Id: <3c8e7b02-2152-495a-a0b6-e37cf9286a70@app.fastmail.com>
In-Reply-To: 
 <CAAJ_b94=gtCeUKkGPUmPj_2SwHV+PiXQ8Mx-1RqfYj3pP3OwpA@mail.gmail.com>
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>
Subject: Re: pg_waldump: support decoding of WAL inside tarfile
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/3c8e7b02-2152-495a-a0b6-e37cf9286a70%40app.fastmail.com>
Precedence: bulk

On Tue, Jan 27, 2026, at 9:06 AM, Amul Sul wrote:
>
> I have included your patch (removing WalSegSz) to the attached patch
> set as 0001; all my subsequent patches are now bumped by one.
>

I started looking at this patch. This is not a complete review.

0002. LGTM.

0003. If this is a refactor, you should move the unlikely logic (if necessary)
to 0005.

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.

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

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

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

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

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

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.

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

+   /* Before that we must parse the tar archive. */
+   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'.)

+   /* Hash table storing WAL entries read from the archive */
+   ArchivedWAL_HTAB = ArchivedWAL_create(16, NULL);

Any reason for 16? Comment says nothing about it.

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

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

+       /*
+        * XXX: If the segment being read not the requested one, the data must
+        * be buffered, as we currently lack the mechanism to write it to a
+        * temporary file. This is a known limitation that will be fixed in the
+        * next patch, as the buffer could grow up to the full WAL segment
+        * size.
+        */
+       if (segno > entry->segno)
+           continue;
+
+       /* WAL segments must be archived in order */
+       pg_log_error("WAL files are not archived in sequential order");
+       pg_log_error_detail("Expecting segment number " UINT64_FORMAT " but found " UINT64_FORMAT ".",
+                           segno, entry->segno);

Can it enforce a specific order? tar follows an arbitrary order in which the
files is returned by the filesystem. You've been debating a solution to buffer
the WAL contents using memory or spilled files. If it always create the tar in
an alphabetical order, you can reduce the scope of this patch. (Didn't look
what challenges are expected to use a sorted list to generate the tar file.)

-  dependencies: [frontend_code, lz4, zstd],
+  dependencies: [frontend_code, lz4, zstd, libpq],

Put libpq after frontend_code.

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

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

my @lines = test_pg_waldump($path);

+/* Forward declaration */
+struct ArchivedWALFile;

Why don't you move ArchivedWALFile to pg_waldump.h? pg_waldump.h is included
into archive_waldump.c.

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

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

  could not change permissions of \"%s\": %m

+   pg_log_debug("temporarily exporting file \"%s\"", fpath);                   

spilling to temporary file \"%s\"

0007. LGTM.

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.

0009. LGTM.


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/