On Mon, Dec 19, 2022 at 12:23 AM Michael Paquier <mich...@paquier.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 <mich...@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.
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