Re: pg_upgrade check for invalid databases

On Fri, 11 Oct 2024 at 04:01, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > On 7 Oct 2024, at 22:04, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> >
> > On Mon, Oct 07, 2024 at 03:37:35PM -0400, Bruce Momjian wrote:
> >> On Tue, Oct 1, 2024 at 09:28:54AM +0200, Daniel Gustafsson wrote:
> >>> Correct, sorry for being unclear. The consistency argument would be to expand
> >>> pg_upgrade to report all invalid databases rather than just the first found;
> >>> attempting to fix problems would be a new behavior.
> >>
> >> Yes, historically pg_upgrade will fail if it finds anything unusual,
> >> mostly because what it does normally is already scary enough. If users
> >> what pg_upgrade to do cleanups, it would be enabled by a separate flag,
> >> or even a new command-line app.
> >
> > While I suspect it's rare that someone CTRL-C's out of an accidental DROP
> > DATABASE and then runs pg_upgrade before trying to recover the data, I
> > agree with the principle of having pg_upgrade fail by default for things
> > like this. If we did add a new flag, the new invalid database report that
> > Daniel mentions could say something like "try again with
> > --skip-invalid-databases to have pg_upgrade automatically drop invalid
> > databases."
>
> If we are teaching pg_upgrade to handle errors, either by skipping or by
> fixing, then I believe this is the right way to go about it. A successful run
> should probably also create a report of the databases which were skipped.

In v2 I've made changes to the patch incorporating the suggestions here:

* Default behaviour is to just fail with a report of all invalid databases

* A new option --skip-invalid-databases will then skip the checks, and
would not transfer any invalid database to the new cluster. A warning
with a report file will then follow after a successful run.

Dropping invalid databases in the old cluster will make invalid
databases unrecoverable, so I opted for a skip over invalid databases
approach that would leave invalid databases in the old cluster.

Apart from a missing --skip-invalid-databases test, does this attempt look OK?