public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: David Christensen <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Date: Thu, 15 Dec 2022 15:36:06 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAOxo6XJMcLyLb1D4oOV2FHfJvt1Z6DuediGUKKEHAEMvpPe2ow@mail.gmail.com>
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>
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 != 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 =
+ qr/^([0-9A-F]{8})-([0-9A-F]{8})[.][0-9]+[.][0-9]+[.][0-9]+[.][0-9]+(?:_vm|_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 permissions
+umask(0077);
Incorrect copy-paste coming from elsewhere like the TAP tests of
pg_basebackup with group permissions? Doesn't
PostgreSQL::Test::Utils::tempdir give already enough protection in
terms of umask() and permissions?
+ if (config.save_fpi_path != NULL)
+ {
+ /* We fsync our output directory only; since these files are not part
+ * 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 = $node->safe_psql('postgres',
+ q{SELECT format('%s/%s/%s', CASE WHEN reltablespace = 0 THEN
dattablespace ELSE reltablespace END, pg_database.oid,
pg_relation_filenode(pg_class.oid)) FROM pg_class, pg_database WHERE
relname = 'test_table' AND datname = 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.
+$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 = 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (55+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox