Re: pg_upgrade should truncate/remove its logs before running

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
cleanup(void)
{
+ int dbnum;
+ char **filename;
+ char filename_path[MAXPGPATH];
[...]
+ if (rmdir(filename_path))
+ pg_log(PG_WARNING, "failed to rmdir: %s: %m\n",
filename_path);
+
+ 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.
--
Michael