Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 15 Dec 2022 15:36:06 +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: <Y5rAViBOVChfrsHN@paquier.xyz>
References: 
 <CALj2ACWTQCYGHf=2pHQeX=8qfkMi-4JsXN0t_6W8+A-cW8wJpw@mail.gmail.com>
 <CAOxo6X+wXEu1QfTFTf-WiDs3jr8ERvgEOddHfUhAH_5eje-G2w@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="pPjI34RbK/8yHj76"
Content-Disposition: inline
In-Reply-To: 
 <CAOxo6XJMcLyLb1D4oOV2FHfJvt1Z6DuediGUKKEHAEMvpPe2ow@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Y5rAViBOVChfrsHN%40paquier.xyz>
Precedence: bulk


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

On Wed, Dec 14, 2022 at 04:44:34PM -0600, David Christensen wrote:
> I can get one sent in tomorrow.

-XLogRecordHasFPW(XLogReaderState *record)
+XLogRecordHasFPI(XLogReaderState *record)
This still refers to a FPW, so let's leave that out as well as any
renamings of this kind..

+   if (config.save_fpi_path !=3D NULL)
+   {
+       /* Create the dir if it doesn't exist */
+       if (pg_mkdir_p(config.save_fpi_path, pg_dir_create_mode) < 0)
+       {
+           pg_log_error("could not create output directory \"%s\": %m",
+                        config.save_fpi_path);
+           goto bad_argument;
+       }
+   }
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.

+my $file_re =3D
+  qr/^([0-9A-F]{8})-([0-9A-F]{8})[.][0-9]+[.][0-9]+[.][0-9]+[.][0-9]+(?:_v=
m|_init|_fsm|_main)?$/;
This is artistic to parse for people not used to regexps (I do, a
little).  Perhaps this could use a small comment with an example of
name or a reference describing this format?

+# Set umask so test directories and files are created with default permiss=
ions
+umask(0077);
Incorrect copy-paste coming from elsewhere like the TAP tests of
pg_basebackup with group permissions?  Doesn't=20
PostgreSQL::Test::Utils::tempdir give already enough protection in
terms of umask() and permissions?

+   if (config.save_fpi_path !=3D NULL)
+   {
+       /* We fsync our output directory only; since these files are not pa=
rt
+        * of the production database we do not require the performance hit
+        * that fsyncing every FPI would entail, so are doing this as a
+        * compromise. */
+
+       fsync_fname(config.save_fpi_path, true);
+   }
Why is it necessary to flush anything at all, then?

+my $relation =3D $node->safe_psql('postgres',
+   q{SELECT format('%s/%s/%s', CASE WHEN reltablespace =3D 0 THEN
dattablespace ELSE reltablespace END, pg_database.oid,
pg_relation_filenode(pg_class.oid)) FROM pg_class, pg_database WHERE
relname =3D 'test_table' AND datname =3D current_database()}
Could you rewrite that to be on multiple lines?

+diag "using walfile: $walfile";
You should avoid the use of "diag", as this would cause extra output
noise with a simple make check.

+$node->safe_psql('postgres', "SELECT pg_drop_replication_slot('regress_pg_=
waldump_slot')")
That's not really necessary, the nodes are wiped out at the end of the
test.=20

+$node->safe_psql('postgres', <<EOF);
+SELECT 'init' FROM pg_create_physical_replication_slot('regress_pg_waldump=
_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.)
--
Michael

--pPjI34RbK/8yHj76
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmOawFYACgkQnvQgOdby
QH2ZnxAAqNn8/oIwH7baBTiXAotKlz8ygG/c7Rjvlxn8NkqhsNA1XjRpvuNXVoTZ
pND9vEYeog3nj6+QJ8VQFXftWU7GE+kL/bFBoKcbJvn6VixtwloKO+UBRwiP074H
ecNgOdDxD9wjQKBPoOnqGaTpeRWXo5F8ECyM4FIVZaGVnH2G8hAQTTuJqp/d9zi3
Z6gdeviMGRlbsXlCggg10ybxibxHvG2fId1BYgg4PwTi/tfDeaDrAXkJ/WBdSUrS
moHShk+YwTIJNZC521RcizXVgdBOU8Pme3wlOUKCCO3tQzofPj/hAhMpSHry7XF2
WBGe3FAyXMuZbK82cwCb9mD7qwpnP1b0pe4rV3kPNJ+dMZOnwqq3l+wncBjPJy7r
KVo2y7HRgL80dxGEBWxgxdcnDPhefLkqSqATovAOLDSn7ZvCwe7dfJtro5k6dZjF
wwdXNeeFz0QJy7plTwZVw5kYbjY8ZLTJTT2tJ8mI2jtuyAQD7tXClrYgDJy20ehf
p2DYG+wWulaSJUnpR+rlyfA0mZIKR0iKj/DSQOBhotkcPdCj6IJuyAUPCzteKhpY
j3eg/yDgy9wSwsZgZ4NQlM8sm/p+J5qYTbzxQDDf8oqIaUgXTTrUplt/GHnOP6jm
MAs/hOVsFHEvavVYvHOWZcfuTQBcVv4weSd0SWn6c7BPNb/pLBE=
=QLmY
-----END PGP SIGNATURE-----

--pPjI34RbK/8yHj76--