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>
In-Reply-To: <CAAJ_b94=gtCeUKkGPUmPj_2SwHV+PiXQ8Mx-1RqfYj3pP3OwpA@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Tue, 27 Jan 2026 16:11:26 -0500
Message-ID: <CA+TgmoYuC-92tMxZHn-KY=ExF-YkHkd4yOfiMhi9upqk3M1DuA@mail.gmail.com>
Subject: Re: pg_waldump: support decoding of WAL inside tarfile
To: Amul Sul <sulamul@gmail.com>
Cc: 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/CA%2BTgmoYuC-92tMxZHn-KY%3DExF-YkHkd4yOfiMhi9upqk3M1DuA%40mail.gmail.com>
Precedence: bulk

On Tue, Jan 27, 2026 at 7:07=E2=80=AFAM Amul Sul <sulamul@gmail.com> wrote:
> In the attached version, I am using the WAL segment name as the hash
> key, which is much more straightforward. I have rewritten
> read_archive_wal_page(), and it looks much cleaner than before. The
> logic to discard irrelevant WAL files is still within
> get_archive_wal_entry. I added an explanation for setting cur_wal to
> NULL, which is now handled in the separate function I mentioned
> previously.
>
> Kindly have a look at the attached version; let me know if you are
> still not happy with the current approach for filtering/discarding
> irrelevant WAL segments. It isn't much different from the previous
> version, but I have tried to keep it in a separate routine for better
> code readability, with comments to make it easier to understand. I
> also added a comment for ArchivedWALFile.

I feel like the division of labor between get_archive_wal_entry() and
read_archive_wal_page() is odd. I noticed this in the last version,
too, and it still seems to be the case. get_archive_wal_entry() first
calls ArchivedWAL_lookup(). If that finds an entry, it just returns.
If it doesn't, it loops until an entry for the requested file shows up
and then returns it. Then control returns to read_archive_wal_page()
which loops some more until we have all the data we need for the
requested file. But it seems odd to me to have two separate loops
here. I think that the first loop is going to call read_archive_file()
until we find the beginning of the file that we care about and then
the second one is going to call read_archive_file() some more until we
have read enough of it to satisfy the request. It feels odd to me to
do it that way, as if we told somebody to first wait until 9 o'clock
and then wait another 30 minutes, instead of just telling them to wait
until 9:30. I realize it's not quite the same thing, because apart
from calling read_archive_file(), the two loops do different things,
but I still think it looks odd.

+ /*
+ * Ignore if the timeline is different or the current segment is not
+ * the desired one.
+ */
+ XLogFromFileName(entry->fname, &curSegTimeline, &curSegNo, WalSegSz);
+ if (privateInfo->timeline !=3D curSegTimeline ||
+ privateInfo->startSegNo > curSegNo ||
+ privateInfo->endSegNo < curSegNo ||
+ segno > curSegNo)
+ {
+ free_archive_wal_entry(entry->fname, privateInfo);
+ continue;
+ }

The comment doesn't match the code. If it did, the test would be
(privateInfo->timeline !=3D curSegTimeline || segno !=3D curSegno). But
instead the segno test is > rather than !=3D, and the checks against
startSegNo and endSegNo aren't explained at all. I think I understand
why the segno test uses > rather than !=3D, but it's the point of the
comment to explain things like that, rather than leaving the reader to
guess. And I don't know why we also need to test startSegNo and
endSegNo.

I also wonder what the point is of doing XLogFromFileName() on the
fname provided by the caller and then again on entry->fname. Couldn't
you just compare the strings?

Again, the division of labor is really odd here. It's the job of
astreamer_waldump_content() to skip things that aren't WAL files at
all, but it's the job of get_archive_wal_entry() to skip things that
are WAL files but not the one we want. I disagree with putting those
checks in completely separate parts of the code.

+static bool
+member_is_wal_file(astreamer_waldump *mystreamer, astreamer_member *member=
,
+    char **fname)
+{
+ int pathlen;
+ char    *filename;
+
+ /* We are only interested in normal files. */
+ if (member->is_directory || member->is_link)
+ return false;
+
+ pathlen =3D strlen(member->pathname);
+ if (pathlen < XLOG_FNAME_LEN)
+ return false;
+
+ /* WAL files from the top-level or pg_wal directory will be decoded */
+ if (pathlen > XLOG_FNAME_LEN &&
+ strncmp(member->pathname, XLOGDIR, strlen(XLOGDIR)) !=3D 0)
+ return false;
+
+ /* WAL file could be with full path */
+ filename =3D member->pathname + (pathlen - XLOG_FNAME_LEN);
+ if (!IsXLogFileName(filename))
+ return false;
+
+ *fname =3D pnstrdup(filename, XLOG_FNAME_LEN);
+
+ return true;
+}

I don't think this code is fully correct. Note that pg_verifybackup's
member_verify_header() prepends "./" and then applies
canonicalize_path(). I think the same considerations apply here.
Consider:

[rhaas pgdata]$ (cd pg_wal; tar -cf ../pg_wal1.tar 00*)
[rhaas pgdata]$ (cd pg_wal; tar -cf ../pg_wal2.tar .)
[rhaas pgdata]$ tar tf pg_wal1.tar
00000001000000000000002C
00000001000000000000002D
00000001000000000000002E
00000001000000000000002F
000000010000000000000030
000000010000000000000031
000000010000000000000032
000000010000000000000033
[rhaas pgdata]$ tar tf pg_wal2.tar
./
./00000001000000000000002C
./00000001000000000000002D
./00000001000000000000002E
./archive_status/
./000000010000000000000031
./000000010000000000000030
./summaries/
./00000001000000000000002F
./000000010000000000000032
./000000010000000000000033

--
Robert Haas
EDB: http://www.enterprisedb.com