Re: pg_upgrade should truncate/remove its logs before running

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
Date: 2022-01-20 03:01:29
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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
weird, anyway:

>> + <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.


Attachment Content-Type Size
v4-0001-pg_upgrade-write-log-files-and-dumps-in-subdir.patch text/x-diff 17.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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