Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL

On Mon, Dec 19, 2022 at 12:23 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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(at)paquier(dot)xyz> wrote:
> > 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.
> >
> > 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.

I guess I'm feeling a little dense here; how is this failing if there
is an existing empty directory?

> >> +$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.
>
> By the way, knowing that we have an option called --fullpage, could be
> be better to use --save-fullpage for the option name?

Works for me. I think I'd just wanted to avoid reformatting the
entire usage message which is why I'd gone with the shorter version.

> + OPF = fopen(filename, PG_BINARY_W);
> + if (!OPF)
> + pg_fatal("couldn't open file for output: %s", filename);
> [..]
> + if (fwrite(page, BLCKSZ, 1, OPF) != 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.

Sure, will update.

Best,

David