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
signature.asc
Description: PGP signature
