Horton

Thread

Commits

  1. pg_upgrade: Move all the files generated internally to a subdirectory

  1. pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-12T02:50:17Z

    I have seen this numerous times but had not dug into it, until now.

If pg_upgrade fails and is re-run, it appends to its logfiles, which is
confusing since, if it fails again, it then looks like the original error
recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.

I think it should either truncate the logfiles, or error early if any of the
files exist.  Or it could put all its output files into a newly-created
subdirectory.  Or this message could be output to the per-db logfiles, and not
just the static ones:
| "pg_upgrade run on %s".

For the per-db logfiels with OIDs in their name, changing open() from "append"
mode to truncate mode doesn't work, since they're written to in parallel.
They have to be removed/truncated in advance.

This is one possible fix.  You can test its effect by deliberately breaking one
of the calls to exec_progs(), like this.

-  "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
+  "\"%s/pg_restore\" %s %s --exit-on-error --verboose "

  2. Re: pg_upgrade should truncate/remove its logs before running

    Bruce Momjian <bruce@momjian.us> — 2021-12-15T21:09:16Z

    On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
> I have seen this numerous times but had not dug into it, until now.
> 
> If pg_upgrade fails and is re-run, it appends to its logfiles, which is
> confusing since, if it fails again, it then looks like the original error
> recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.
> 
> I think it should either truncate the logfiles, or error early if any of the
> files exist.  Or it could put all its output files into a newly-created
> subdirectory.  Or this message could be output to the per-db logfiles, and not
> just the static ones:
> | "pg_upgrade run on %s".
> 
> For the per-db logfiels with OIDs in their name, changing open() from "append"
> mode to truncate mode doesn't work, since they're written to in parallel.
> They have to be removed/truncated in advance.
> 
> This is one possible fix.  You can test its effect by deliberately breaking one
> of the calls to exec_progs(), like this.
> 
> -  "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
> +  "\"%s/pg_restore\" %s %s --exit-on-error --verboose "

Uh, the database server doesn't erase its logs on crash/failure, so why
should pg_upgrade do that?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  3. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-15T21:12:12Z

    On Wed, Dec 15, 2021 at 04:09:16PM -0500, Bruce Momjian wrote:
> On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
> > I have seen this numerous times but had not dug into it, until now.
> > 
> > If pg_upgrade fails and is re-run, it appends to its logfiles, which is
> > confusing since, if it fails again, it then looks like the original error
> > recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.
> > 
> > I think it should either truncate the logfiles, or error early if any of the
> > files exist.  Or it could put all its output files into a newly-created
> > subdirectory.  Or this message could be output to the per-db logfiles, and not
> > just the static ones:
> > | "pg_upgrade run on %s".
> > 
> > For the per-db logfiels with OIDs in their name, changing open() from "append"
> > mode to truncate mode doesn't work, since they're written to in parallel.
> > They have to be removed/truncated in advance.
> > 
> > This is one possible fix.  You can test its effect by deliberately breaking one
> > of the calls to exec_progs(), like this.
> > 
> > -  "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
> > +  "\"%s/pg_restore\" %s %s --exit-on-error --verboose "
> 
> Uh, the database server doesn't erase its logs on crash/failure, so why
> should pg_upgrade do that?

To avoid the presence of irrelevant errors from the previous invocation of
pg_upgrade.

Maybe you would prefer one of my other ideas , like "put all its output files
into a newly-created subdirectory" ?

-- 
Justin

  4. Re: pg_upgrade should truncate/remove its logs before running

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-15T21:17:23Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
>> If pg_upgrade fails and is re-run, it appends to its logfiles, which is
>> confusing since, if it fails again, it then looks like the original error
>> recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.

> Uh, the database server doesn't erase its logs on crash/failure, so why
> should pg_upgrade do that?

The server emits enough information so that it's not confusing:
there are timestamps, and there's an identifiable startup line.
pg_upgrade does neither.  If you don't want to truncate as
Justin suggests, you should do that instead.

Personally I like the idea of making a timestamped subdirectory
and dropping all the files in that, because the thing that most
annoys *me* about pg_upgrade is the litter it leaves behind in
$CWD.  A subdirectory would make it far easier to mop up the mess.

			regards, tom lane

  5. Re: pg_upgrade should truncate/remove its logs before running

    Bruce Momjian <bruce@momjian.us> — 2021-12-15T21:23:43Z

    On Wed, Dec 15, 2021 at 04:17:23PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
> >> If pg_upgrade fails and is re-run, it appends to its logfiles, which is
> >> confusing since, if it fails again, it then looks like the original error
> >> recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.
> 
> > Uh, the database server doesn't erase its logs on crash/failure, so why
> > should pg_upgrade do that?
> 
> The server emits enough information so that it's not confusing:
> there are timestamps, and there's an identifiable startup line.
> pg_upgrade does neither.  If you don't want to truncate as
> Justin suggests, you should do that instead.
> 
> Personally I like the idea of making a timestamped subdirectory
> and dropping all the files in that, because the thing that most
> annoys *me* about pg_upgrade is the litter it leaves behind in
> $CWD.  A subdirectory would make it far easier to mop up the mess.

Yes, lot of litter.  Putting it in a subdirectory makes a lot of sense.
Justin, do you want to work on that patch, since you had an earlier
version to fix this?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  6. Re: pg_upgrade should truncate/remove its logs before running

    Andrew Dunstan <andrew@dunslane.net> — 2021-12-15T22:04:54Z

    On 12/15/21 16:23, Bruce Momjian wrote:
> On Wed, Dec 15, 2021 at 04:17:23PM -0500, Tom Lane wrote:
>> Bruce Momjian <bruce@momjian.us> writes:
>>> On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
>>>> If pg_upgrade fails and is re-run, it appends to its logfiles, which is
>>>> confusing since, if it fails again, it then looks like the original error
>>>> recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.
>>> Uh, the database server doesn't erase its logs on crash/failure, so why
>>> should pg_upgrade do that?
>> The server emits enough information so that it's not confusing:
>> there are timestamps, and there's an identifiable startup line.
>> pg_upgrade does neither.  If you don't want to truncate as
>> Justin suggests, you should do that instead.
>>
>> Personally I like the idea of making a timestamped subdirectory
>> and dropping all the files in that, because the thing that most
>> annoys *me* about pg_upgrade is the litter it leaves behind in
>> $CWD.  A subdirectory would make it far easier to mop up the mess.
> Yes, lot of litter.  Putting it in a subdirectory makes a lot of sense.
> Justin, do you want to work on that patch, since you had an earlier
> version to fix this?
>



The directory name needs to be predictable somehow, or maybe optionally
set as a parameter. Having just a timestamped directory name would make
life annoying for a poor buildfarm maintainer. Also, please don't change
anything before I have a chance to adjust the buildfarm code to what is
going to be done.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  7. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-15T22:13:10Z

    On Wed, Dec 15, 2021 at 05:04:54PM -0500, Andrew Dunstan wrote:
> On 12/15/21 16:23, Bruce Momjian wrote:
> > On Wed, Dec 15, 2021 at 04:17:23PM -0500, Tom Lane wrote:
> >> Bruce Momjian <bruce@momjian.us> writes:
> >>> On Sat, Dec 11, 2021 at 08:50:17PM -0600, Justin Pryzby wrote:
> >>>> If pg_upgrade fails and is re-run, it appends to its logfiles, which is
> >>>> confusing since, if it fails again, it then looks like the original error
> >>>> recurred and wasn't fixed.  The "append" behavior dates back to 717f6d608.
> >>> Uh, the database server doesn't erase its logs on crash/failure, so why
> >>> should pg_upgrade do that?
> >> The server emits enough information so that it's not confusing:
> >> there are timestamps, and there's an identifiable startup line.
> >> pg_upgrade does neither.  If you don't want to truncate as
> >> Justin suggests, you should do that instead.
> >>
> >> Personally I like the idea of making a timestamped subdirectory
> >> and dropping all the files in that, because the thing that most
> >> annoys *me* about pg_upgrade is the litter it leaves behind in
> >> $CWD.  A subdirectory would make it far easier to mop up the mess.
> > Yes, lot of litter.  Putting it in a subdirectory makes a lot of sense.
> > Justin, do you want to work on that patch, since you had an earlier
> > version to fix this?
> 
> The directory name needs to be predictable somehow, or maybe optionally
> set as a parameter. Having just a timestamped directory name would make
> life annoying for a poor buildfarm maintainer. Also, please don't change
> anything before I have a chance to adjust the buildfarm code to what is
> going to be done.

Feel free to suggest the desirable behavior.
It could write to pg_upgrade.log/* and refuse to run if the dir already exists.

-- 
Justin

  8. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2021-12-16T01:39:05Z

    On Wed, Dec 15, 2021 at 04:13:10PM -0600, Justin Pryzby wrote:
> On Wed, Dec 15, 2021 at 05:04:54PM -0500, Andrew Dunstan wrote:
>> The directory name needs to be predictable somehow, or maybe optionally
>> set as a parameter. Having just a timestamped directory name would make
>> life annoying for a poor buildfarm maintainer. Also, please don't change
>> anything before I have a chance to adjust the buildfarm code to what is
>> going to be done.
> 
> Feel free to suggest the desirable behavior.
> It could write to pg_upgrade.log/* and refuse to run if the dir already exists.

Andrew's point looks rather sensible to me.  So, this stuff should
have a predictable name (pg_upgrade.log, pg_upgrade_log or upgrade_log
would be fine).  But I would also add an option to be able to define a
custom log path.  The latter would be useful for the regression tests
so as everything gets could get redirected to a path already filtered
out.
--
Michael

  9. Re: pg_upgrade should truncate/remove its logs before running

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-16T11:11:25Z

    On 16.12.21 02:39, Michael Paquier wrote:
> On Wed, Dec 15, 2021 at 04:13:10PM -0600, Justin Pryzby wrote:
>> On Wed, Dec 15, 2021 at 05:04:54PM -0500, Andrew Dunstan wrote:
>>> The directory name needs to be predictable somehow, or maybe optionally
>>> set as a parameter. Having just a timestamped directory name would make
>>> life annoying for a poor buildfarm maintainer. Also, please don't change
>>> anything before I have a chance to adjust the buildfarm code to what is
>>> going to be done.
>>
>> Feel free to suggest the desirable behavior.
>> It could write to pg_upgrade.log/* and refuse to run if the dir already exists.
> 
> Andrew's point looks rather sensible to me.  So, this stuff should
> have a predictable name (pg_upgrade.log, pg_upgrade_log or upgrade_log
> would be fine).  But I would also add an option to be able to define a
> custom log path.  The latter would be useful for the regression tests
> so as everything gets could get redirected to a path already filtered
> out.

Could we make it write just one log file?  Is having multiple log files 
better?

  10. Re: pg_upgrade should truncate/remove its logs before running

    Daniel Gustafsson <daniel@yesql.se> — 2021-12-16T11:23:08Z

    > On 16 Dec 2021, at 12:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> Could we make it write just one log file?  Is having multiple log files better?

Having individual <checkname>.txt files from checks with additional information
on how to handle the error are quite convenient when writing wrappers around
pg_upgrade (speaking from experience of having written multiple pg_upgraade
frontends).  Parsing a single logfile is more work, and will break existing
scripts.

I'm in favor of a predictable by default logpath, with a parameter to override,
as mentioned upthread.

--
Daniel Gustafsson		https://vmware.com/

  11. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-17T17:21:13Z

    On Thu, Dec 16, 2021 at 12:23:08PM +0100, Daniel Gustafsson wrote:
> > On 16 Dec 2021, at 12:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> > Could we make it write just one log file?  Is having multiple log files better?
> 
> Having individual <checkname>.txt files from checks with additional information
> on how to handle the error are quite convenient when writing wrappers around
> pg_upgrade (speaking from experience of having written multiple pg_upgraade
> frontends).  Parsing a single logfile is more work, and will break existing
> scripts.
> 
> I'm in favor of a predictable by default logpath, with a parameter to override,
> as mentioned upthread.

I put this together in the simplest way, prefixing all the filenames with the
configured path..

Another options is to chdir() into the given path.  But, pg_upgrade takes (and
requires) a bunch of other paths, like -d -D -b -B, and those are traditionally
interpretted relative to CWD.  I could getcwd() and prefix all the -[dDbB] with
that, but prefixing a handful of binary/data paths is hardly better than
prefixing a handful of dump/logfile paths.  I suppose that openat() isn't
portable.  I don't think this it's worth prohibiting relative paths, so I can't
think of any less-naive way to do this.

I didn't move the delete-old-cluster.sh, since that's intended to stay around
even after a successful upgrade, as opposed to the other logs, which are
typically removed at that point.

-- 
Justin

  12. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2021-12-20T11:21:51Z

    On Fri, Dec 17, 2021 at 11:21:13AM -0600, Justin Pryzby wrote:
> I put this together in the simplest way, prefixing all the filenames with the
> configured path..

Well, why not.

> Another options is to chdir() into the given path.  But, pg_upgrade takes (and
> requires) a bunch of other paths, like -d -D -b -B, and those are traditionally
> interpretted relative to CWD.  I could getcwd() and prefix all the -[dDbB] with
> that, but prefixing a handful of binary/data paths is hardly better than
> prefixing a handful of dump/logfile paths.  I suppose that openat() isn't
> portable.  I don't think this it's worth prohibiting relative paths, so I can't
> think of any less-naive way to do this.

If we add a new file, .gitignore would find about it quickly and
inform about a not-so-clean tree.  I would tend to prefer your
approach, here.  Relative paths can be useful.

> I didn't move the delete-old-cluster.sh, since that's intended to stay around
> even after a successful upgrade, as opposed to the other logs, which are
> typically removed at that point.

Makes sense to me.

+       log_opts.basedir = getenv("PG_UPGRADE_LOGDIR");
+       if (log_opts.basedir != NULL)
+               log_opts.basedir = strdup(log_opts.basedir);
+       else
+               log_opts.basedir = "pg_upgrade_log.d";
Why is this controlled with an environment variable?  It seems to me
that an option switch would be much better, no?  While tuning things,
we could choose something simpler for the default, like
"pg_upgrade_log".  I don't have a good history in naming new things,
though :)

.gitignore should be updated, I guess?  Besides, this patch has no
documentation.
--
Michael

  13. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-21T03:39:26Z

    On Mon, Dec 20, 2021 at 08:21:51PM +0900, Michael Paquier wrote:
> On Fri, Dec 17, 2021 at 11:21:13AM -0600, Justin Pryzby wrote:

> +               log_opts.basedir = "pg_upgrade_log.d";

> we could choose something simpler for the default, like
> "pg_upgrade_log".  I don't have a good history in naming new things,
> though :)

I specifically called it .d to made it obvious that it's a dir - nearly
everything that ends in "log" is a file, so people are likely to run "rm" and
"less" on it - including myself.

> .gitignore should be updated, I guess?

Are you suggesting to remove these ?
-/pg_upgrade_internal.log
-/reindex_hash.sql
-/loadable_libraries.txt

> Besides, this patch has no documentation.

TBH I'm not even sure if the dir needs to be configurable ?

  14. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2021-12-22T07:47:12Z

    On Mon, Dec 20, 2021 at 09:39:26PM -0600, Justin Pryzby wrote:
> On Mon, Dec 20, 2021 at 08:21:51PM +0900, Michael Paquier wrote:
>> we could choose something simpler for the default, like
>> "pg_upgrade_log".  I don't have a good history in naming new things,
>> though :)
> 
> I specifically called it .d to made it obvious that it's a dir - nearly
> everything that ends in "log" is a file, so people are likely to run "rm" and
> "less" on it - including myself.

Okay.

>> .gitignore should be updated, I guess?
> 
> Are you suggesting to remove these ?
> -/pg_upgrade_internal.log
> -/loadable_libraries.txt

Yep, it looks so as these are part of the logs, the second one being a
failure state.

> -/reindex_hash.sql

But this one is not, no?

>> Besides, this patch has no documentation.
> 
> TBH I'm not even sure if the dir needs to be configurable ?

I'd think it is better to have some control on that.  Not sure what
the opinion of others is on this specific point, though.
--
Michael

  15. Re: pg_upgrade should truncate/remove its logs before running

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-22T14:52:26Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Dec 20, 2021 at 09:39:26PM -0600, Justin Pryzby wrote:
>> Are you suggesting to remove these ?
>> -/pg_upgrade_internal.log
>> -/loadable_libraries.txt

> Yep, it looks so as these are part of the logs, the second one being a
> failure state.

>> -/reindex_hash.sql

> But this one is not, no?

I'd like to get to a state where there's just one thing to "rm -rf"
to clean up after any pg_upgrade run.  If we continue to leave the
we-suggest-you-run-these scripts loose in $CWD then we've not really
improved things much.

Perhaps there'd be merit in putting log files into an additional
subdirectory of that output directory, like
pg_upgrade_output.d/logs/foo.log, so that the more-ignorable
output files would be separated from the less-ignorable ones.
Or perhaps that's just gilding the lily.

			regards, tom lane

  16. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-22T16:36:12Z

    On Wed, Dec 22, 2021 at 09:52:26AM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Mon, Dec 20, 2021 at 09:39:26PM -0600, Justin Pryzby wrote:
> >> Are you suggesting to remove these ?
> >> -/pg_upgrade_internal.log
> >> -/loadable_libraries.txt
> 
> > Yep, it looks so as these are part of the logs, the second one being a
> > failure state.
> 
> >> -/reindex_hash.sql
> 
> > But this one is not, no?
> 
> I'd like to get to a state where there's just one thing to "rm -rf"
> to clean up after any pg_upgrade run.  If we continue to leave the
> we-suggest-you-run-these scripts loose in $CWD then we've not really
> improved things much.

My patch moves reindex_hash.sql, and I'm having trouble seeing why it shouldn't
be handled in .gitignore the same way as other stuff that's moved.

But delete-old-cluster.sh is not moved, and I'm not sure how to improve on
that.

> Perhaps there'd be merit in putting log files into an additional
> subdirectory of that output directory, like
> pg_upgrade_output.d/logs/foo.log, so that the more-ignorable
> output files would be separated from the less-ignorable ones.
> Or perhaps that's just gilding the lily.

In the case it's successful, everything is removed - except for the delete
script.  I can see the case for separating the dumps (which are essentially
internal and of which there may be many) and the logs (same), from
the .txt error files like loadable_libraries.txt (which are user-facing).

It could also be divided with each DB having its own subdir, with a dumpfile
and a logfile.

Should the unix socket be created underneath the "output dir" ?

Should it be possible to set the output dir to "." ?  That would give the
pre-existing behavior, but only if we don't use subdirs for log/ and dump/.

  17. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-08T18:48:57Z

    The cfbot was failing under windows:
| [22:07:02.159] could not create directory "pg_upgrade_output.d": File exists

It's because parseCommandLine() was called before get_restricted_token(), which
re-executes the process, and runs parseCommandLine again.

parseCommandLine already does stuff like opening logfiles, so that's where my
mkdir() is.  It fails when re-run, since the re-exec doesn't call the cleanup()
path.

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.

BTW the CI integration is pretty swell.  I added a few lines of debugging code
to figure out what was happening here.  check world on 4 OSes is faster than
check world run locally.  I rearranged cirrus.yaml to make windows run its
upgrade check first to save a few minutes.

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?

-- 
Justin

  18. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-11T07:41:58Z

    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

  19. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-11T20:03:07Z

    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

  20. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-12T03:59:54Z

    On Tue, Jan 11, 2022 at 02:03:07PM -0600, Justin Pryzby wrote:
> 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 ?

Yes, something like that.

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

This is a path for the data internal to pg_upgrade.  My take is that
the code simplifications the new option brings are more valuable than
this assumption, which I guess would unlikely happen.  I may be wrong,
of course.  By the way, while thinking about that, should we worry
about --logdir="."?
--
Michael

  21. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-12T04:08:13Z

    On Wed, Jan 12, 2022 at 12:59:54PM +0900, Michael Paquier wrote:
> On Tue, Jan 11, 2022 at 02:03:07PM -0600, Justin Pryzby wrote:
> > 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.
> 
> This is a path for the data internal to pg_upgrade.  My take is that
> the code simplifications the new option brings are more valuable than
> this assumption, which I guess would unlikely happen.  I may be wrong,
> of course.  By the way, while thinking about that, should we worry
> about --logdir="."?

I asked about that before.  Right now, it'll exit(1) when mkdir fails.

I had written a patch to allow "." by skipping mkdir (or allowing it to fail if
errno == EEXIST), but it seems like an awfully bad idea to try to make that
work with rmtree().

-- 
Justin

  22. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-19T08:13:18Z

    On Tue, Jan 11, 2022 at 10:08:13PM -0600, Justin Pryzby wrote:
> I asked about that before.  Right now, it'll exit(1) when mkdir fails.
> 
> I had written a patch to allow "." by skipping mkdir (or allowing it to fail if
> errno == EEXIST), but it seems like an awfully bad idea to try to make that
> work with rmtree().

So, I have been poking at this patch, and found myself doing a couple
of modifications:
- Renaming of the option from --logdir to --outputdir, as this does
not include only logs.  That matches also better with default value
assigned in previous patches, aka pg_upgrade_output.d.
- Convert the output directory to an absolute path when the various
directories are created, and use that for the whole run.  pg_upgrade
is unlikely going to chdir(), but I don't really see why we should
just not use an absolute path all the time, set from the start.
- Add some sanity check about the path used, aka no parent reference
allowed and the output path should not be a direct parent of the
current working directory.
- Rather than assuming that "log/" and "dump/" are hardcoded in
various places, save more paths into log_opts.

I have noticed a couple of incorrect things in the docs, and some
other things.  It is a bit late here, so I may have missed a couple of
things but I'll look at this stuff once again in a couple of days.

So, what do you think?
--
Michael

  23. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-19T10:39:41Z

    On Wed, Jan 19, 2022 at 05:13:18PM +0900, Michael Paquier wrote:
> I have noticed a couple of incorrect things in the docs, and some
> other things.  It is a bit late here, so I may have missed a couple of
> things but I'll look at this stuff once again in a couple of days.

And the docs failed to build..
--
Michael

  24. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-20T00:05:40Z

    On Wed, Jan 19, 2022 at 05:13:18PM +0900, Michael Paquier wrote:
> On Tue, Jan 11, 2022 at 10:08:13PM -0600, Justin Pryzby wrote:
> > I asked about that before.  Right now, it'll exit(1) when mkdir fails.
> > 
> > I had written a patch to allow "." by skipping mkdir (or allowing it to fail if
> > errno == EEXIST), but it seems like an awfully bad idea to try to make that
> > work with rmtree().

I still don't know if it even needs to be configurable.

> - Add some sanity check about the path used, aka no parent reference
> allowed and the output path should not be a direct parent of the
> current working directory.

I'm not sure these restrictions are needed ?

+       outputpath = make_absolute_path(log_opts.basedir);                                                                                          
+       if (path_contains_parent_reference(outputpath))                                                                                             
+               pg_fatal("reference to parent directory not allowed\n");                                                                            

Besides, you're passing the wrong path here.

> I have noticed a couple of incorrect things in the docs, and some
> other things.  It is a bit late here, so I may have missed a couple of
> things but I'll look at this stuff once again in a couple of days.

> +      <command>pg_upgrade</command>, and is be removed after a successful

remove "be"

> +       if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))

S_IRWXG | S_IRWXO are useless due to the umask, right ?
Maybe use PG_DIR_MODE_OWNER ?

> +       if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))
> +               pg_fatal("could not create directory \"%s\": %m\n", filename_path);
> +       if (mkdir(log_opts.dumpdir, S_IRWXU | S_IRWXG | S_IRWXO))
> +               pg_fatal("could not create directory \"%s\": %m\n", filename_path);
> +       if (mkdir(log_opts.logdir, S_IRWXU | S_IRWXG | S_IRWXO))
> +               pg_fatal("could not create directory \"%s\": %m\n", filename_path);

You're printing the wrong var.  filename_path is not initialized.

-- 
Justin

  25. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-20T03:01:29Z

    On Wed, Jan 19, 2022 at 06:05:40PM -0600, Justin Pryzby wrote:
> I still don't know if it even needs to be configurable.

I want this to be configurable to ease the switch of the pg_upgrade to
TAP, moving the logs into a deterministic temporary location proper to
each run.  This makes reporting much easier on failure, with
repeatable tests, and that's why I began poking at this patch first.

> I'm not sure these restrictions are needed ?

This could lead to issues with rmtree() if we are not careful enough,
no?  We'd had our deal of argument injections with pg_upgrade commands
in the past (fcd15f1).

> +       outputpath = make_absolute_path(log_opts.basedir);                                                                                          
> +       if (path_contains_parent_reference(outputpath))                                                                                             
> +               pg_fatal("reference to parent directory not allowed\n");                                                                            
> 
> Besides, you're passing the wrong path here.

What would you suggest?  I was just looking at that again this
morning, and splitted the logic into two parts for the absolute and
relative path cases, preventing all cases like that, which would be
weird, anyway:
../
../popo
.././
././
/direct/path/to/cwd/
/direct/path/../path/to/cwd/

>> +      <command>pg_upgrade</command>, and is be removed after a successful
> 
> remove "be"

Fixed.

>> +       if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))
> 
> S_IRWXG | S_IRWXO are useless due to the umask, right ?
> Maybe use PG_DIR_MODE_OWNER ?

Hmm.  We could just use pg_dir_create_mode, then.  See pg_rewind, as
one example.  This opens the door for something pluggable to
SetDataDirectoryCreatePerm(), though the original use is kind of
different with data folders.

> You're printing the wrong var.  filename_path is not initialized.

Ugh.
--
Michael

  26. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-20T03:59:14Z

    On Thu, Jan 20, 2022 at 12:01:29PM +0900, Michael Paquier wrote:
> On Wed, Jan 19, 2022 at 06:05:40PM -0600, Justin Pryzby wrote:
> 
> > I'm not sure these restrictions are needed ?
> 
> This could lead to issues with rmtree() if we are not careful enough,
> no?  We'd had our deal of argument injections with pg_upgrade commands
> in the past (fcd15f1).

We require that the dir not exist, by testing if (mkdir()).
So it's okay if someone specifies ../whatever or $CWD.

-- 
Justin

  27. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-20T04:38:45Z

    On Wed, Jan 19, 2022 at 09:59:14PM -0600, Justin Pryzby wrote:
> We require that the dir not exist, by testing if (mkdir()).
> So it's okay if someone specifies ../whatever or $CWD.

What I am scared of here is the use of rmtree() if we allow something
like that.  So we should either keep the removal code in its original
shape and allow such cases, or restrict the output path.  At the end,
something has to change.  My points are in favor of the latter because
I don't really see anybody doing the former.  You favor the former.
Now, we are not talking about a lot of code for any of these, anyway.
Perhaps we'd better wait for more opinions.
--
Michael

  28. Re: pg_upgrade should truncate/remove its logs before running

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-20T09:31:15Z

    On 19.01.22 09:13, Michael Paquier wrote:
> - Renaming of the option from --logdir to --outputdir, as this does
> not include only logs.  That matches also better with default value
> assigned in previous patches, aka pg_upgrade_output.d.

I'm afraid that is too easily confused with the target directory. 
Generally, a tool processes data from input to output or from source to 
target or something like that, whereas a log is more clearly something 
separate from this main processing stream.  The desired "output" of 
pg_upgrade is the upgraded cluster, after all.

A wildcard idea is to put the log output into the target cluster.

  29. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-20T10:51:37Z

    On Thu, Jan 20, 2022 at 10:31:15AM +0100, Peter Eisentraut wrote:
> I'm afraid that is too easily confused with the target directory. Generally,
> a tool processes data from input to output or from source to target or
> something like that, whereas a log is more clearly something separate from
> this main processing stream.  The desired "output" of pg_upgrade is the
> upgraded cluster, after all.
> 
> A wildcard idea is to put the log output into the target cluster.

Neat idea.  That would work fine for my case.  So I am fine to stick
with this suggestion. 
--
Michael

  30. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-24T01:59:40Z

    On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> Neat idea.  That would work fine for my case.  So I am fine to stick
> with this suggestion. 

I have been looking at this idea, and the result is quite nice, being
simpler than anything that has been proposed on this thread yet.  We
get a simpler removal logic, and there is no need to perform any kind
of sanity checks with the output path provided as long as we generate
the paths and the dirs after adjust_data_dir().

Thoughts?
--
Michael

  31. Re: pg_upgrade should truncate/remove its logs before running

    Bruce Momjian <bruce@momjian.us> — 2022-01-24T17:39:30Z

    On Mon, Jan 24, 2022 at 10:59:40AM +0900, Michael Paquier wrote:
> On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> > Neat idea.  That would work fine for my case.  So I am fine to stick
> > with this suggestion. 
> 
> I have been looking at this idea, and the result is quite nice, being
> simpler than anything that has been proposed on this thread yet.  We
> get a simpler removal logic, and there is no need to perform any kind
> of sanity checks with the output path provided as long as we generate
> the paths and the dirs after adjust_data_dir().
...
>  
>    <para>
>     <application>pg_upgrade</application> creates various working files, such
> -   as schema dumps, in the current working directory.  For security, be sure
> -   that that directory is not readable or writable by any other users.
> +   as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
> +   the directory of the new cluster.
>    </para>

Uh, how are we instructing people to delete that pg_upgrade output
directory?  If pg_upgrade completes cleanly, would it be removed
automatically?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  32. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-24T17:41:17Z

    On Mon, Jan 24, 2022 at 12:39:30PM -0500, Bruce Momjian wrote:
> On Mon, Jan 24, 2022 at 10:59:40AM +0900, Michael Paquier wrote:
> > On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> > > Neat idea.  That would work fine for my case.  So I am fine to stick
> > > with this suggestion. 
> > 
> > I have been looking at this idea, and the result is quite nice, being
> > simpler than anything that has been proposed on this thread yet.  We
> > get a simpler removal logic, and there is no need to perform any kind
> > of sanity checks with the output path provided as long as we generate
> > the paths and the dirs after adjust_data_dir().
> ...
> >  
> >    <para>
> >     <application>pg_upgrade</application> creates various working files, such
> > -   as schema dumps, in the current working directory.  For security, be sure
> > -   that that directory is not readable or writable by any other users.
> > +   as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
> > +   the directory of the new cluster.
> >    </para>
> 
> Uh, how are we instructing people to delete that pg_upgrade output
> directory?  If pg_upgrade completes cleanly, would it be removed
> automatically?

Clearly.

@@ -689,28 +751,5 @@ cleanup(void)                                                                                                                  
                                                                                                                                                    
        /* Remove dump and log files? */                                                                                                            
        if (!log_opts.retain)                                                                                                                       
-       {                                                                                                                                           
-               int                     dbnum;                                                                                                      
-               char      **filename;                                                                                                               
-                                                                                                                                                   
-               for (filename = output_files; *filename != NULL; filename++)                                                                        
-                       unlink(*filename);                                                                                                          
-                                                                                                                                                   
-               /* remove dump files */                                                                                                             
-               unlink(GLOBALS_DUMP_FILE);                                                                                                          
-                                                                                                                                                   
-               if (old_cluster.dbarr.dbs)                                                                                                          
-                       for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)                                                                    
-                       {                                                                                                                           
-                               char            sql_file_name[MAXPGPATH],                                                                           
-                                                       log_file_name[MAXPGPATH];                                                                   
-                               DbInfo     *old_db = &old_cluster.dbarr.dbs[dbnum];                                                                 
-                                                                                                                                                   
-                               snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);                                  
-                               unlink(sql_file_name);                                                                                              
-                                                                                                                                                   
-                               snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);                              
-                               unlink(log_file_name);                                                                                              
-                       }                                                                                                                           
-       }                                                                                                                                           
+               rmtree(log_opts.basedir, true);                                                                                                     
 }

  33. Re: pg_upgrade should truncate/remove its logs before running

    Bruce Momjian <bruce@momjian.us> — 2022-01-24T19:44:21Z

    On Mon, Jan 24, 2022 at 11:41:17AM -0600, Justin Pryzby wrote:
> On Mon, Jan 24, 2022 at 12:39:30PM -0500, Bruce Momjian wrote:
> > On Mon, Jan 24, 2022 at 10:59:40AM +0900, Michael Paquier wrote:
> > > On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> > > > Neat idea.  That would work fine for my case.  So I am fine to stick
> > > > with this suggestion. 
> > > 
> > > I have been looking at this idea, and the result is quite nice, being
> > > simpler than anything that has been proposed on this thread yet.  We
> > > get a simpler removal logic, and there is no need to perform any kind
> > > of sanity checks with the output path provided as long as we generate
> > > the paths and the dirs after adjust_data_dir().
> > ...
> > >  
> > >    <para>
> > >     <application>pg_upgrade</application> creates various working files, such
> > > -   as schema dumps, in the current working directory.  For security, be sure
> > > -   that that directory is not readable or writable by any other users.
> > > +   as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
> > > +   the directory of the new cluster.
> > >    </para>
> > 
> > Uh, how are we instructing people to delete that pg_upgrade output
> > directory?  If pg_upgrade completes cleanly, would it be removed
> > automatically?
> 
> Clearly.

OK, thanks.  There are really two cleanups --- first, the "log"
directory, and second deletion of the old cluster by running
delete_old_cluster.sh.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

  34. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-24T22:53:28Z

    On Mon, Jan 24, 2022 at 02:44:21PM -0500, Bruce Momjian wrote:
> OK, thanks.  There are really two cleanups --- first, the "log"
> directory, and second deletion of the old cluster by running
> delete_old_cluster.sh.

Yes, this is the same thing as what's done on HEAD with a two-step
cleanup, except that we just only need to remove the log directory
rather than each individual log entry.
--
Michael

  35. Re: pg_upgrade should truncate/remove its logs before running

    Justin Pryzby <pryzby@telsasoft.com> — 2022-01-25T16:45:29Z

    On Mon, Jan 24, 2022 at 10:59:40AM +0900, Michael Paquier wrote:
> On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> > Neat idea.  That would work fine for my case.  So I am fine to stick
> > with this suggestion. 
> 
> I have been looking at this idea, and the result is quite nice, being
> simpler than anything that has been proposed on this thread yet.  We
> get a simpler removal logic, and there is no need to perform any kind
> of sanity checks with the output path provided as long as we generate
> the paths and the dirs after adjust_data_dir().
> 
> Thoughts?

Andrew: you wanted to accommodate any change on the build client, right ?

-- 
Justin

  36. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-26T00:44:48Z

    On Tue, Jan 25, 2022 at 10:45:29AM -0600, Justin Pryzby wrote:
> Andrew: you wanted to accommodate any change on the build client, right ?

Yes, this is going to need an adjustment of @logfiles in
TestUpgrade.pm, with the addition of
"$tmp_data_dir/pg_update_output.d/log/*.log" to be consistent with the
data fetched for the tests of older branches.
--
Michael

  37. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-26T02:00:28Z

    On Wed, Jan 26, 2022 at 09:44:48AM +0900, Michael Paquier wrote:
> Yes, this is going to need an adjustment of @logfiles in
> TestUpgrade.pm, with the addition of
> "$tmp_data_dir/pg_update_output.d/log/*.log" to be consistent with the
> data fetched for the tests of older branches.

Bleh.  This would point to the old data directory, so this needs to be
"$self->{pgsql}/src/bin/pg_upgrade/tmp_check/data/pg_upgrade_output.d/log/*.log"
to point to the upgraded cluster.
--
Michael

  38. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-28T13:42:15Z

    On Wed, Jan 26, 2022 at 11:00:28AM +0900, Michael Paquier wrote:
> Bleh.  This would point to the old data directory, so this needs to be
> "$self->{pgsql}/src/bin/pg_upgrade/tmp_check/data/pg_upgrade_output.d/log/*.log"
> to point to the upgraded cluster.

Please note that I have sent a patch to merge this change in the
buildfarm code.  Comments are welcome.
--
Michael

  39. Re: pg_upgrade should truncate/remove its logs before running

    Andrew Dunstan <andrew@dunslane.net> — 2022-01-28T23:27:29Z

    On 1/28/22 08:42, Michael Paquier wrote:
> On Wed, Jan 26, 2022 at 11:00:28AM +0900, Michael Paquier wrote:
>> Bleh.  This would point to the old data directory, so this needs to be
>> "$self->{pgsql}/src/bin/pg_upgrade/tmp_check/data/pg_upgrade_output.d/log/*.log"
>> to point to the upgraded cluster.
> Please note that I have sent a patch to merge this change in the
> buildfarm code.  Comments are welcome.



I have committed this. But it will take time to get every buildfarm own
to upgrade. I will try to make a new release ASAP.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  40. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-01-29T00:53:25Z

    On Fri, Jan 28, 2022 at 06:27:29PM -0500, Andrew Dunstan wrote:
> I have committed this. But it will take time to get every buildfarm own
> to upgrade.

Thanks for that.

> I will try to make a new release ASAP.

And thanks for that, as well.
--
Michael

  41. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-02-06T04:36:07Z

    On Sat, Jan 29, 2022 at 09:53:25AM +0900, Michael Paquier wrote:
> On Fri, Jan 28, 2022 at 06:27:29PM -0500, Andrew Dunstan wrote:
>> I have committed this. But it will take time to get every buildfarm own
>> to upgrade.
> 
> Thanks for that.

So, it took me some time to get back to this thread, and looked at it
for the last couple of days...  The buildfarm client v14 has been
released on the 29th of January, which means that we are good to go.

I have found one issue while reviewing things: the creation of the new
subdirectory and its contents should satisfy group permissions for the
new cluster's data folder, but we were not doing that properly as we
called GetDataDirectoryCreatePerm() after make_outputdirs() so we
missed the proper values for create_mode and umask().  The rest looked
fine, and I got a green CI run on my own repo.  Hence, applied.

I'll keep an eye on the buildfarm, in case.
--
Michael

  42. Re: pg_upgrade should truncate/remove its logs before running

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-06T06:03:44Z

    Hi,

On Sun, Feb 06, 2022 at 01:36:07PM +0900, Michael Paquier wrote:
> 
> The buildfarm client v14 has been
> released on the 29th of January, which means that we are good to go.

I didn't follow that thread closely, but if having the latest buildfarm client
version installed is a hard requirement this will likely be a problem.  First,
there was no email to warn buildfarm owners that a new version is available,
and even if there was I doubt that every owner would have updated it since.
Especially since this is the lunar new year period, so at least 2 buildfarm
owners (me included) are on holidays since last week.

  43. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-02-06T06:11:44Z

    On Sun, Feb 06, 2022 at 02:03:44PM +0800, Julien Rouhaud wrote:
> I didn't follow that thread closely, but if having the latest buildfarm client
> version installed is a hard requirement this will likely be a problem.  First,
> there was no email to warn buildfarm owners that a new version is available,
> and even if there was I doubt that every owner would have updated it since.
> Especially since this is the lunar new year period, so at least 2 buildfarm
> owners (me included) are on holidays since last week.

The buildfarm will still be able to work as it did so that's not a
hard requirement per-se.  The only thing changing is that we would not
find the logs in the event of a failure in the tests of pg_upgrade,
and the buildfarm client is coded to never fail if it does not see
logs in some of the paths it looks at, it just holds the full history
of the paths we have used across the ages.
--
Michael

  44. Re: pg_upgrade should truncate/remove its logs before running

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-06T06:58:21Z

    Michael Paquier <michael@paquier.xyz> writes:
> So, it took me some time to get back to this thread, and looked at it
> for the last couple of days...  The buildfarm client v14 has been
> released on the 29th of January, which means that we are good to go.

As already mentioned, there's been no notice to buildfarm owners ...
so has Andrew actually made a release?

			regards, tom lane

  45. Re: pg_upgrade should truncate/remove its logs before running

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-06T07:03:11Z

    On Sun, Feb 06, 2022 at 01:58:21AM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > So, it took me some time to get back to this thread, and looked at it
> > for the last couple of days...  The buildfarm client v14 has been
> > released on the 29th of January, which means that we are good to go.
> 
> As already mentioned, there's been no notice to buildfarm owners ...
> so has Andrew actually made a release?

There's a v14 release on the github project ([1]) from 8 days ago, so it seems
so.

[1] https://github.com/PGBuildFarm/client-code/releases/tag/REL_14

  46. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-02-06T07:04:54Z

    On Sun, Feb 06, 2022 at 01:58:21AM -0500, Tom Lane wrote:
> As already mentioned, there's been no notice to buildfarm owners ...
> so has Andrew actually made a release?

There has been one as of 8 days ago:
https://github.com/PGBuildFarm/client-code/releases

And I have just looked at that as point of reference.
--
Michael

  47. Re: pg_upgrade should truncate/remove its logs before running

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-06T07:17:48Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Feb 06, 2022 at 01:58:21AM -0500, Tom Lane wrote:
>> As already mentioned, there's been no notice to buildfarm owners ...
>> so has Andrew actually made a release?

> There has been one as of 8 days ago:
> https://github.com/PGBuildFarm/client-code/releases

[ scrapes buildfarm logs ... ]

Not even Andrew's own buildfarm critters are using it, so
permit me leave to doubt that he thinks it's fully baked.

Andrew?

			regards, tom lane

  48. Re: pg_upgrade should truncate/remove its logs before running

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-06T13:32:59Z

    On 2/6/22 02:17, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Sun, Feb 06, 2022 at 01:58:21AM -0500, Tom Lane wrote:
>>> As already mentioned, there's been no notice to buildfarm owners ...
>>> so has Andrew actually made a release?
>> There has been one as of 8 days ago:
>> https://github.com/PGBuildFarm/client-code/releases
> [ scrapes buildfarm logs ... ]
>
> Not even Andrew's own buildfarm critters are using it, so
> permit me leave to doubt that he thinks it's fully baked.
>
> Andrew?
>
> 			


*sigh* Sometimes I have a mind like a sieve. I prepped the release a few
days ago and meant to come back the next morning and send out emails
announcing it, as well as rolling it out to my animals, and got diverted
so that didn't happen and it slipped my mind. I'll go and do those
things now.

But the commit really shouldn't have happened until we know that most
buildfarm owners have installed it. It should have waited wait not just
for the release but for widespread deployment. Otherwise we will just
lose any logging for an error that might appear.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  49. Re: pg_upgrade should truncate/remove its logs before running

    Michael Paquier <michael@paquier.xyz> — 2022-02-07T00:33:13Z

    On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote:
> *sigh* Sometimes I have a mind like a sieve. I prepped the release a few
> days ago and meant to come back the next morning and send out emails
> announcing it, as well as rolling it out to my animals, and got diverted
> so that didn't happen and it slipped my mind. I'll go and do those
> things now.

Thanks.  I saw the release listed after a couple of days of
hibernation, and that one week went by since, so I thought that the
timing was pretty good.  I did not check the buildfarm members though,
sorry about that.

> But the commit really shouldn't have happened until we know that most
> buildfarm owners have installed it. It should have waited wait not just
> for the release but for widespread deployment. Otherwise we will just
> lose any logging for an error that might appear.

Would it be better if I just revert the change for now then and do it
again in one/two weeks?  The buildfarm is green, so keeping things as 
they are does not sound like a huge deal to me, either, for this
case.

FWIW, I have already switched my own animal to use the newest
buildfarm client.
--
Michael

  50. Re: pg_upgrade should truncate/remove its logs before running

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-07T00:39:39Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote:
>> But the commit really shouldn't have happened until we know that most
>> buildfarm owners have installed it. It should have waited wait not just
>> for the release but for widespread deployment. Otherwise we will just
>> lose any logging for an error that might appear.

> Would it be better if I just revert the change for now then and do it
> again in one/two weeks?

I don't see a need to revert it.

I note, though, that there's still not been any email to the buildfarm
owners list about this update.

			regards, tom lane

  51. Re: pg_upgrade should truncate/remove its logs before running

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-07T00:47:32Z

    
> On Feb 6, 2022, at 7:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Michael Paquier <michael@paquier.xyz> writes:
>>> On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote:
>>> But the commit really shouldn't have happened until we know that most
>>> buildfarm owners have installed it. It should have waited wait not just
>>> for the release but for widespread deployment. Otherwise we will just
>>> lose any logging for an error that might appear.
> 
>> Would it be better if I just revert the change for now then and do it
>> again in one/two weeks?
> 
> I don't see a need to revert it.
> 
> I note, though, that there's still not been any email to the buildfarm
> owners list about this update.
> 
>  

It’s stuck in moderation 

Cheers

Andrew

  52. Re: pg_upgrade should truncate/remove its logs before running

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-07T16:00:22Z

    On 2/6/22 19:39, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote:
>>> But the commit really shouldn't have happened until we know that most
>>> buildfarm owners have installed it. It should have waited wait not just
>>> for the release but for widespread deployment. Otherwise we will just
>>> lose any logging for an error that might appear.
>> Would it be better if I just revert the change for now then and do it
>> again in one/two weeks?
> I don't see a need to revert it.
>
> I note, though, that there's still not been any email to the buildfarm
> owners list about this update.
>
> 			


The announcement was held up in list moderation for 20 hours or so.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  53. Re: pg_upgrade should truncate/remove its logs before running

    Julien Rouhaud <rjuju123@gmail.com> — 2022-02-07T16:31:45Z

    On Mon, Feb 07, 2022 at 11:00:22AM -0500, Andrew Dunstan wrote:
> 
> On 2/6/22 19:39, Tom Lane wrote:
> >
> > I note, though, that there's still not been any email to the buildfarm
> > owners list about this update.
> 
> The announcement was held up in list moderation for 20 hours or so.

I've certainly experienced way more than that in the past.  Are volunteers
needed?