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-11 07:41:58
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Jan 08, 2022 at 12:48:57PM -0600, Justin Pryzby wrote:
> I fixed it by calling get_restricted_token() before parseCommandLine().
> There's precedent for that in pg_regress (but the 3 other callers do it
> differently).
> It seems more ideal to always call get_restricted_token sooner than later, but
> for now I only changed pg_upgrade. It's probably also better if
> parseCommandLine() only parses the commandline, but for now I added on to the
> logfile stuff that's already there.
Well, the routine does a bit more than just parsing the options as it
creates the directory infrastructure as well. As you say, I think
that it would be better to have the option parsing and the
loading-into-structure portions in one routine, and the creation of
the paths in a second one. So, the new contents of the patch could
just be moved in a new routine, after getting the restricted token.
Moving get_restricted_token() before or after the option parsing as
you do is not a big deal, but your patch is introducing in the
existing routine more than what's currently done there as of HEAD.

> Maybe the commandline argument should be callled something other than "logdir"
> since it also outputs dumps there. But the dumps are more or less not
> user-facing. But -d and -o are already used. Maybe it shouldn't be
> configurable at all?

If the choice of a short option becomes confusing, I'd be fine with
just a long option, but -l is fine IMO. Including the internal dumps
in the directory is fine to me, and using a subdir, as you do, makes
things more organized.

- "--binary-upgrade %s -f %s",
+ "--binary-upgrade %s -f %s/dump/%s",
Some quotes seem to be missing here.

static void
+ int dbnum;
+ char **filename;
+ char filename_path[MAXPGPATH];
+ if (rmdir(filename_path))
+ pg_log(PG_WARNING, "failed to rmdir: %s: %m\n",
+ if (rmdir(log_opts.basedir))
+ pg_log(PG_WARNING, "failed to rmdir: %s: %m\n", log_opts.basedir);

Is it intentional to not use rmtree() here? If you put all the data
in the same directory, cleanup() gets simpler.

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Sushant Postgres 2022-01-11 07:49:10 Re: [Ext:] Re: Stream Replication not working
Previous Message Michael Paquier 2022-01-11 07:14:25 Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set