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>
In-Reply-To: <CAAJ_b97VUiP-DbLNe-ddq64J_RiB4ZcPgAjHkJH-0dbzgjR++A@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 19 Jan 2026 08:30:48 -0500
Message-ID: <CA+TgmoYMtcZBaqy9r59eDapaDy3WOdepkFFURu9MV-x-kxEbKg@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%2BTgmoYMtcZBaqy9r59eDapaDy3WOdepkFFURu9MV-x-kxEbKg%40mail.gmail.com>
Precedence: bulk

On Mon, Jan 19, 2026 at 6:14=E2=80=AFAM Amul Sul <sulamul@gmail.com> wrote:
> We previously tried having the astreamer function perform all
> necessary checks (e.g., TLI and segment number), but this became
> problematic in init_archive_reader when we needed to read a single WAL
> page just to determine the WAL segment size without any validation. To
> bypass those checks, I initially used a READ_ANY_WAL() macro, but it
> felt like a hack.
>
> In the current design, the archive streamer is responsible only for
> reading and copying data to the buffer. To optimize this and avoid
> unnecessary memcpy and  data buffering for WAL files that aren't
> needed, we set privateInfo->cur_wal to NULL, signaling the streamer
> code to skip those operations.

Hmm. In that case, the code definitely deserves a comment, as that was
not at all clear to me.

> But, I am thinking that instead of setting privateInfo->cur_wal to
> NULL, we could simply discard the buffer data at that place and let
> memcpy in astreamer_content copy if it would be that much of an issue.

I don't think doing unnecessary copying is the right way forward. The
copying itself could be expensive, but I am a little concerned about
the memory utilization of this code. Suppose the user has increased
the WAL segment size to 1GB or even higher. It seems like we could
buffer a whole segment or maybe even more in some scenarios. If we
avoid copying data we don't need, then we also avoid buffering it in
memory.

In terms of the separation of concerns, we could view setting
privateInfo->cur_wal =3D NULL as a form of signaling, a way for this
code to tell the astreamer that it doesn't need the data buffering.
However, I think it might be better to make the signaling more
explicit. Instead of having the caller directly set the buffer to
NULL, or directly trim data out of the buffer, maybe it should set
some value in privateInfo that tells the astreamer what to do. For
instance, suppose it sets the oldest LSN that it might still care
about in privateInfo, and then the astreamer is free to do skip
copying of any data prior to that LSN, and discard any that it already
has. Especially if properly commented, I think this might be more
clear than what you have now. I am not 100% sure that's the right
idea, though. I just think right now it's a bit murky who does what
and why the division of responsibilities is what it is.

For example, read_archive_wal_page() currently has some responsibility
for discarding data, but is that because it is the right place to
discard data, or is that just because that's where we have an LSN
available that tells us what we can discard? I don't know, but an
explicit signaling mechanism like what I describe in the previous
paragraph can make it possible for those two things to happen in
different places.

> In the earlier version, we had only one buffer where we copied
> streamed data, but a problem (previously discussed [1]) arose when the
> archive streamer reads data from two WAL files -- i.e., one is ending
> and the next is starting. We have to keep track of where the previous
> file ends and the next one starts, along with the WAL segment
> information that each belongs to.

Hmm, interesting. My first thought was that if we were starting a new
WAL file, isn't it time to flush the data from the old one to a spill
file? But maybe it's not, if the WAL reader can reread records, and a
record can span file boundaries. But probably we need some comment so
that the next reader is not confused, and we definitely need to make
sure that the structure members in ArchivedWALEntry are well-chosen
and well-documented.

> > +       if (strstr(member->pathname, "PaxHeaders."))
> > +               return false;
> >
> > There is no way that a filename containing the string "PaxHeaders."
> > could ever pass the IsXLogFileName test just above. We shouldn't need
> > this.
> >
>
> This checks the directory name (e.g.,
> x_dir/y_dir/PaxHeaders.NNNN/wal_file_name). The name of that metadata
> file is exactly the same as the WAL file name, which is why
> IsXLogFileName() doesn't help here.

I think this code should only be considering files in the toplevel
directory, and skipping over any directories it finds. I absolutely
promise I am not going to commit anything that is specifically looking
for PaxHeaders. Nothing we've ever done with tar files up to now has
required that, and I don't think this should, either.

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