|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Justin Pryzby <pryzby(at)telsasoft(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Bruce Momjian <bruce(at)momjian(dot)us>, pgsql-hackers(at)lists(dot)postgresql(dot)org|
|Subject:||Re: pg_upgrade should truncate/remove its logs before running|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Wed, Jan 19, 2022 at 06:05:40PM -0600, Justin Pryzby wrote:
> I still don't know if it even needs to be configurable.
I want this to be configurable to ease the switch of the pg_upgrade to
TAP, moving the logs into a deterministic temporary location proper to
each run. This makes reporting much easier on failure, with
repeatable tests, and that's why I began poking at this patch first.
> I'm not sure these restrictions are needed ?
This could lead to issues with rmtree() if we are not careful enough,
no? We'd had our deal of argument injections with pg_upgrade commands
in the past (fcd15f1).
> + outputpath = make_absolute_path(log_opts.basedir);
> + if (path_contains_parent_reference(outputpath))
> + pg_fatal("reference to parent directory not allowed\n");
> Besides, you're passing the wrong path here.
What would you suggest? I was just looking at that again this
morning, and splitted the logic into two parts for the absolute and
relative path cases, preventing all cases like that, which would be
>> + <command>pg_upgrade</command>, and is be removed after a successful
> remove "be"
>> + if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))
> S_IRWXG | S_IRWXO are useless due to the umask, right ?
> Maybe use PG_DIR_MODE_OWNER ?
Hmm. We could just use pg_dir_create_mode, then. See pg_rewind, as
one example. This opens the door for something pluggable to
SetDataDirectoryCreatePerm(), though the original use is kind of
different with data folders.
> You're printing the wrong var. filename_path is not initialized.
|Next Message||Andres Freund||2022-01-20 03:12:26||Re: Add last commit LSN to pg_last_committed_xact()|
|Previous Message||Kyotaro Horiguchi||2022-01-20 03:00:56||Re: Null commitTS bug|