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