Re: [v15 beta] pg_upgrade failed if earlier executed with -c switch

On Mon, Jun 06, 2022 at 02:38:03AM +0200, Daniel Gustafsson wrote:
> On 5 Jun 2022, at 11:19, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> On Sun, Jun 05, 2022 at 09:24:25AM +0900, Michael Paquier wrote:
>>> Well, another error that could happen in the early code paths is
>>> EACCES on a custom socket directory specified, and we'd still face the
>>> same problem on a follow-up restart. Using a sub-directory structure
>>> as Daniel and Tom mention would address all that (if ignoring EEXIST
>>> for the BASE_OUTPUTDIR), removing any existing content from the base
>>> path when not using --retain. This comes with the disadvantage of
>>> bloating the disk on repeated errors, but this last bit would not
>>> really be a huge problem, I guess, as it could be more useful to keep
>>> the error information around.
>>
>> I have been toying with the idea of a sub-directory named with a
>> timestamp (Unix time, like log_line_prefix's %n but this could be
>> any format) under pg_upgrade_output.d/ and finished with the
>> attached.
>
> I was thinking more along the lines of %m to make it (more) human readable, but
> I'm certainly not wedded to any format.

Neither am I. I would not map exactly to %m as it uses whitespaces,
but something like %Y%m%d_%H%M%S.%03d (3-digit ms for last part) would
be fine? If there are other ideas for the format, just let me know.

> As a user I would expect the logs from this current invocation to be removed
> without --retain, and any other older log entries be kept. I think we should
> remove log_opts.logdir and only remove log_opts.rootdir if it is left empty
> after .logdir is removed.

Okay, however I think you mean log_opts.basedir rather than logdir?
That's simple enough to switch around as pg_check_dir() does this
job.

>> The logic in charge of cleaning up the logs has been moved to a single
>> routine, aka cleanup_logs().
>
> + cleanup_logs();
>
> Maybe we should register cleanup_logs() as an atexit() handler once we're done
> with option processing?

It seems to me that the original intention is to keep the logs around
on failure, hence we should only clean up things on a clean exit().
That's why I didn't add an exit callback for that.

> + snprintf(log_opts.logdir, MAXPGPATH, "%s/%s/%s", log_opts.rootdir,
> + timebuf, LOG_OUTPUTDIR);
>
> While not introduced by this patch, it does make me uneasy that we create paths
> without checking for buffer overflows..

I don't mind adding such checks in those code paths. You are right
that they tend to produce longer path strings than others.
--
Michael