Date: Tue, 26 Apr 2022 11:53:54 +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: <Ymdewu9M1LABRzhz@paquier.xyz>
References: 
 <CAOxo6XKjQb2bMSBRpePf3ZpzfNTwjQUc4Tafh21=jzjX6bX8CA@mail.gmail.com>
 <CAEze2Wi_WsKqmCZV0e85eS64TdtgcdANDXTH+xUUD54om2i5hw@mail.gmail.com>
 <CAOxo6XLFd9YreWODAb3K17REPNy2VrOH9-x30mcKGEZMvqKYXA@mail.gmail.com>
 <YmY7eMgqycsWExQt@paquier.xyz>
 <CAOxo6X+zVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="BrHDe2OHu8jn8djw"
Content-Disposition: inline
In-Reply-To: 
 <CAOxo6X+zVNft6qg-EzDy-dM2XYUbccZ-5K5iadJWDf-Ui4dCWQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Ymdewu9M1LABRzhz%40paquier.xyz>
Precedence: bulk


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

On Mon, Apr 25, 2022 at 10:11:10AM -0500, David Christensen wrote:
> On Mon, Apr 25, 2022 at 1:11 AM Michael Paquier <michael@paquier.xyz> wro=
te:
>> 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.
>=20
> Yeah, though I am tending towards what another user had suggested and
> just accepting an existing directory rather than requiring it to be
> empty, so thinking I might just skip this one. (Will review the file
> you've pointed out to see if there is a relevant function though.)

OK.  FWIW, pg_check_dir() is used in initdb and pg_basebackup because
these care about the behavior to use when specifying a target path.
You could reuse it, but use a different policy depending on its
returned result for the needs of what you see fit in this case.

>> +                   PageSetLSN(page, record->ReadRecPtr);
>> +                   /* if checksum field is non-zero then we have checks=
ums enabled,
>> +                    * so recalculate the checksum with new LSN (yes, th=
is 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.
>=20
> Can make a mention; you thinking just a comment in the code is
> sufficient, or want there to be user-visible docs as well?

I was thinking about a comment, at least.

> This was to make the page as extracted equivalent to the effect of
> applying the WAL record block on replay (the LSN and checksum both);
> since recovery calls this function to mark the LSN where the page came
> from this is why I did this as well.  (I do see perhaps a case for
> --raw output that doesn't munge the page whatsoever, just made
> comparisons easier.)

Hm.  Okay.  The argument goes both ways, I guess, depending on what we
want to do with those raw pages.  Still you should not need pd_lsn if
the point is to be able to stick the pages back in place to attempt to
get back as much data as possible when loading it back to the shared
buffers?
--
Michael

--BrHDe2OHu8jn8djw
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJnXsIACgkQnvQgOdby
QH0tUQ/9HYewq+Fzbr16YjUpNBibaCfR0JFg+TaTC1YTawk7K3IYb5l4rGYJ9WNP
C9nlxmx3xOJ9p4VjNMPkCgnaZvCSdRb6X70p8yDFO91nkvhGMm6GtG075bv41ccY
EF5KUn2TgYczfWZnG2jmv7Q9aUXiMGZhbgp359NyrDtLTIKMsKFmxeksgOiITaWj
0h/CIIXxDzcO8xzuz+6v1uwN1DC2O22PvFlknm3bHhkNMKJbRDQyksgFTEt5wcVR
5TSARV8o2VHGfFxQ5zHTsPbmfIoI95EtfByFJO9bqe4DuAW06avFl7EaQlwlfUMY
+NmEyDqRJyf9R38T5u2FhiFZ3fCJI/FQyhie74lRFlP7oe5W2wrRVy7AyuuzOGQ+
I6uaFrkfzlDtu+aQvXQcQcDeEMhJdPac5Jacg3MYA+XlLSjwNp28XoV21b1k3Eb/
dykonezhsbN+udrktwY0lW8BmerMCnGAA84QpTwlYO8tDZFP2N9uKkRkHg48VeP3
dwjloiT7CNaExGqysVfsHQOMVefglXyOiT5bioQPo2zhqmLppvLraN9SuzSksEOP
RhuBMv2fg10FuuqEqL4pPhnOy+bvwpdlb4IZMH2piSs4CF1sToBFf2K7oUpowBZb
YjrMdZ6EWuv4wWN+/7w7SrFhZJghDs68a4BqaX7SeiSMZjTQ144=
=cSXy
-----END PGP SIGNATURE-----

--BrHDe2OHu8jn8djw--