Re: A recent message added to pg_upgade

On Mon, Nov 13, 2023 at 10:19 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Nov 13, 2023 at 08:45:12AM +0530, Amit Kapila wrote:
> > I think we can be specific about logical replication stuff. I have not
> > done any study on autovacuum behavior related to this, so we can
> > update about it separately if required.
>
> Autovacuum, as far as I recall, could decide to do some work before
> files are physically copied from the old to the new cluster,
> corrupting the new cluster. Per 76dd09bbec89:
>
> + * If we have lost the autovacuum launcher, try to start a new one.
> + * We don't want autovacuum to run in binary upgrade mode because
> + * autovacuum might update relfrozenxid for empty tables before
> + * the physical files are put in place.
>
> > - /* Use -b to disable autovacuum. */
> > + /*
> > + * Use -b to disable autovacuum and logical replication launcher
> > + * (effective in PG17 or later for the latter).
> > + *
> > + * Logical replication workers can stream data during the
> > upgrade which can
> > + * cause replication origins to move forward after we have copied them.
> > + * It can cause the system to request the data which is already present
> > + * in the new cluster.
> > + */
> >
> > Now, ideally, such a comment change makes more sense along with the
> > main patch, so either we can go without this comment change or
> > probably wait till the main patch is ready and merge just before it or
> > along with it. I am fine either way.
>
> Another location would be to document that stuff directly in
> launcher.c where the check for IsBinaryUpgrade would be added. You
> are right that it makes little sense to document that now, so how
> about:
> 1) keeping pg_upgrade.c minimal, say:
> - /* Use -b to disable autovacuum. */
> + /*
> + * Use -b to disable autovacuum and logical replication
> + * launcher (in 17~).
> + */
> With a removal of the comment block related to
> max_logical_replication_workers=0?
> 2) Document that in ApplyLauncherRegister() as part of the main patch
> for the subscribers?
>

I am fine with this but there is no harm in doing this before or along
with the main patch. As of now, I don't see any problem but as the
main patch is still under review, so thought we could even wait for
the patch to become "Ready For Committer".

--
With Regards,
Amit Kapila.