Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 19 Dec 2022 15:22:59 +0900
From: Michael Paquier <michael@paquier.xyz>
To: David Christensen <david.christensen@crunchydata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Message-ID: <Y6ADQ83RzfNP5d3o@paquier.xyz>
References: 
 <CAOxo6XJ03w=PWDnsnzDpGK+4Er7XZwFkhqiMbVWmt7VLHzidHw@mail.gmail.com>
 <CALj2ACUukW74XXb2n3FFAdRiisF06jSNDR8DegOpMevq_0yVoA@mail.gmail.com>
 <CAOxo6XJ+uN-kiCS9wVKm2LudZXEZOk6zzduF-uGc_4464uXs9w@mail.gmail.com>
 <CAOxo6X+ALOqqZ+a1r-=BTDaUB6LL0w7nnu=K_kvYbGsrkU2eyA@mail.gmail.com>
 <CALj2ACV4donjBLaxkJ1FO3Ybg73ut--9TozHTsNyBVCdX9opYA@mail.gmail.com>
 <CAOxo6XLhduEAn0yLh+BGOjpq6k0x9MsOY6PjDEoCh4VvdUxCiQ@mail.gmail.com>
 <CALj2ACWmgbDeCDMaZah0GRwQBCJMD01U4S69V6TE4W2jYmnLuQ@mail.gmail.com>
 <CAOxo6XJMcLyLb1D4oOV2FHfJvt1Z6DuediGUKKEHAEMvpPe2ow@mail.gmail.com>
 <Y5rAViBOVChfrsHN@paquier.xyz>
 <CAOxo6X+NCVAB3H5_TMSasgimf127KURoi=S0dTSNJUn8Z8+WNA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="q834xFlEFiG7OiLc"
Content-Disposition: inline
In-Reply-To: 
 <CAOxo6X+NCVAB3H5_TMSasgimf127KURoi=S0dTSNJUn8Z8+WNA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Y6ADQ83RzfNP5d3o%40paquier.xyz>
Precedence: bulk


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

On Thu, Dec 15, 2022 at 05:17:46PM -0600, David Christensen wrote:
> On Thu, Dec 15, 2022 at 12:36 AM Michael Paquier <michael@paquier.xyz> wr=
ote:
> This v10 should incorporate your feedback as well as Bharath's.

Thanks for the new version.  I have minor comments.

>> It seems to me that you could allow things to work even if the
>> directory exists and is empty.  See for example
>> verify_dir_is_empty_or_create() in pg_basebackup.c.
>=20
> The `pg_mkdir_p()` supports an existing directory (and I don't think
> we want to require it to be empty first), so this only errors when it
> can't create a directory for some reason.

Sure, but things can also be made so as we don't fail if the directory
exists and is empty?  This would be more consistent with the base
directories created by pg_basebackup and initdb.

>> +$node->safe_psql('postgres', <<EOF);
>> +SELECT 'init' FROM pg_create_physical_replication_slot('regress_pg_wald=
ump_slot', true, false);
>> +CREATE TABLE test_table AS SELECT generate_series(1,100) a;
>> +CHECKPOINT; -- required to force FPI for next writes
>> +UPDATE test_table SET a =3D a + 1;
>> Using an EOF to execute a multi-line query would be a first.  Couldn't
>> you use the same thing as anywhere else?  009_twophase.pl just to
>> mention one.  (Mentioned by Bharath upthread, where he asked for an
>> extra opinion so here it is.)
>=20
> Fair enough, while idiomatic perl to me, not a hill to die on;
> converted to a standard multiline string.

By the way, knowing that we have an option called --fullpage, could be
be better to use --save-fullpage for the option name?

+       OPF =3D fopen(filename, PG_BINARY_W);
+       if (!OPF)
+           pg_fatal("couldn't open file for output: %s", filename);
[..]
+       if (fwrite(page, BLCKSZ, 1, OPF) !=3D 1)
+           pg_fatal("couldn't write out complete full page image to file: =
%s", filename);
These should more more generic, as of "could not open file \"%s\"" and
"could not write file \"%s\"" as the file name provides all the
information about what this writes.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmOgA0MACgkQnvQgOdby
QH2Xsg//YjDLEhD1yvKhvii2/QehLFVBxAgYcZsEpsv/9FH2kp4McNHh6PF6w3yU
BvymzYZnbDCXSBC2EeFT92K5s41CwbeLfBGqZFC+wW1cORmweekn5CaPwtMJdGIn
f5AV9SW95saqxxs62jGBZJgv9I2I4MB0RAT4KAs3wVpn5btpRolYFTajVWquOXeH
2LNW0E1gWFQVSVq8myVaLftYT4+502dg5Waim/W3QIMMLWy9utybeXfAI8f5+8bJ
9DYkH6ruCibMKjVvkuVnaWUrEeOnk2yAfjp2lPzULC00vhRO6doWTdSrL6cHrelI
BO0J03z/y3A3zJFiuHNnFC0A3sc/c2X3YV+jl5YMMNGtZ9Uz8PWxRP4lQAhj2Q3R
qtCFtOgH4uEWJv4vR/teyL9+rTZjsM1JQCTqroKFL3fqv2CYmuzvSMn37bybzKAD
XPobCHesk5sY8skaCUwF8HrqvwTb1yQnQBGizjflH47lmwSQ9duWvqZDBc2yjnFF
TV8vOs6qQXfBNevjSd9D6ZtSmxXf6xfFitykGBXY2Un5ZRNuNIDPoDEZqlJuCZ4Y
dRz8IBqtOjfQTMp/8zrl6wyjr9dU9wE3PgQld8NNrWkBJHq9PT+dvr1A43XmrLl8
EPhmxZDK9KWXRhW79zZkRqCnTgIdDfJU9mNHclcy6VaDCtMUy8o=
=ycLE
-----END PGP SIGNATURE-----

--q834xFlEFiG7OiLc--