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> 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.
>> +$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?
+ 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.
--
Michael
