On Thu, Dec 15, 2022 at 12:36 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Wed, Dec 14, 2022 at 04:44:34PM -0600, David Christensen wrote: > > I can get one sent in tomorrow.
This v10 should incorporate your feedback as well as Bharath's.
> -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..
Reverted those changes.
> + 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.
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.
> +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?
Added a description of what this is looking for.
> +# 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?
I'd expect that's where that came from. Removed.
> + 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?
I personally don't think it is, but added it per Bharath's request.
Removed in this revision.
> +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?
Sure, reformated.
> +diag "using walfile: $walfile";
> You should avoid the use of "diag", as this would cause extra output
> noise with a simple make check.
Had been using it for debugging and didn't realize it'd cause issues.
Removed both instances.
> +$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.
Removed.
> +$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.)
Fair enough, while idiomatic perl to me, not a hill to die on;
converted to a standard multiline string.
Best,
David
v10-0001-Teach-pg_waldump-to-extract-FPIs-from-the-WAL-st.patch
Description: Binary data
