Date: Mon, 25 Apr 2022 15:11:04 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Christensen <david.christensen@crunchydata.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Message-ID: <YmY7eMgqycsWExQt@paquier.xyz>
References: 
 <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
 <CAEze2Wi_WsKqmCZV0e85eS64TdtgcdANDXTH+xUUD54om2i5hw@mail.gmail.com>
 <CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="SeZJp3N+wCSTpveK"
Content-Disposition: inline
In-Reply-To: 
 <CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YmY7eMgqycsWExQt%40paquier.xyz>
Precedence: bulk


--SeZJp3N+wCSTpveK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 23, 2022 at 01:43:36PM -0500, David Christensen wrote:
> Hi Matthias, great point.  Enclosed is a revised version of the patch
> that adds the fork identifier to the end if it's a non-main fork.

Like Alvaro, I have seen cases where this would have been really
handy.  So +1 from me, as well, to have more tooling like what you are
proposing.  Fine for me to use one file for each block with a name
like what you are suggesting for each one of them.=20

+       /* we accept an empty existing directory */
+       if (stat(config.save_fpw_path, &st) =3D=3D 0 && S_ISDIR(st.st_mode))
+       {
I don't think that there is any need to rely on a new logic if there
is already some code in place able to do the same work.  See
verify_dir_is_empty_or_create() in pg_basebackup.c, as one example,
that relies on pg_check_dir().  I think that you'd better rely at
least on what pgcheckdir.c offers.

+       {"raw-fpi", required_argument, NULL, 'W'},
I think that we'd better rename this option.  "fpi", that is not used
much in the user-facing docs, is additionally not adapted when we have
an other option called -w/--fullpage.  I can think of
--save-fullpage.

+                   PageSetLSN(page, record->ReadRecPtr);
+                   /* if checksum field is non-zero then we have checksums=
 enabled,
+                    * so recalculate the checksum with new LSN (yes, this =
is a hack)
+                    */
Yeah, that looks like a hack, but putting in place a page on a cluster
that has checksums enabled would be more annoying with
zero_damaged_pages enabled if we don't do that, so that's fine by me
as-is.  Perhaps you should mention that FPWs don't have their
pd_checksum updated when written.

+       /* we will now extract the fullpage image from the XLogRecord and s=
ave
+        * it to a calculated filename */
The format of this comment is incorrect.

+            <entry>The LSN of the record with this block, formatted
+            as <literal>%08x-%08X</literal> instead of the
+            conventional <literal>%X/%X</literal> due to filesystem naming
+            limits</entry>
The last part of the sentence about %X/%X could just be removed.  That
could be confusing, at worse.

+                   PageSetLSN(page, record->ReadRecPtr);
Why is pd_lsn set?

git diff --check complains a bit.

This stuff should include some tests.  With --end, the tests can
be cheap.
--
Michael

--SeZJp3N+wCSTpveK
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJmO3gACgkQnvQgOdby
QH0ITA/9FCrzEFCQtsMOTrMX1SJ3ILIOGfx3kohOUsxpOgEUh+U6zji8IuExMJ9E
7CY7nMVtRdhqP1r8L1Kh+ZezIU1wipO8ikPUKa5chX0TkW39prZA5TvG5iFbdj1n
/hqQoQvt5HAAJENbkreoRXm2Zchu86l5SZtyMbbcfH82ifd3+cAreFhlbm9H0yzR
Kukg5aSI+lyzVjO1AQDMhvT7n+hyOgChBUwtX9WMbOtGByl6NILwyEKrWMn0yaoP
u5IyVOgD0G2x80GwWafWa0eYO/KuTY9vldwJfdg5Mx0pPPA9OykJKWIhlW2MA3FY
lNh8G69qym7fe5y66qX+Xgmmn7SbSKIMT1GwKPHcLV13WJpLH70yS3/5b0ltzHMi
0kyxxqnVk1SDjObk1fm+eSt6lrtdSyccHyNh34QXHXrln3CHJAldbrvYtc59qYGs
XDqgxEKqcxBhu1Ahu3weTaSzjQ4bzoQEZfnoHTra/FYm23WBj85fmmqN7AVcfofS
cisn8iiTuzHw/sCMmZnmcI4mHWOFIeBr2XAtMKpTbXCFsU86bn3l2aQRLjsbWp8i
jHICjclH6cohRfwcAXvO7R7Arm433PIvR6+Qww2MtIhfDJDIp5ei3gflRasnYCyp
ErH4IIFrBLQjOoxSGyw+1+JOy7SuvZsH4xVX0C1/d4qbRO4Axnc=
=vHIo
-----END PGP SIGNATURE-----

--SeZJp3N+wCSTpveK--