Re: pg_upgrade should truncate/remove its logs before running

On Tue, Jan 11, 2022 at 04:41:58PM +0900, Michael Paquier wrote:
> 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.

I added mkdir() before the other stuff that messes with logfiles, because it
needs to happen before that.

Are you suggesting to change the pre-existing behavior of when logfiles are
created, like 0002 ?

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

Yes, good catch

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

There's no reason not to. We created the dir, and the user didn't specify to
preserve it. It'd be their fault if they put something valuable there after
starting pg_upgrade.

--
Justin