Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Thu, 19 Oct 2023 at 16:14, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! New patch can be available in [1].
>
> >
> > Few comments:
> > 1) We will be able to override the value of max_slot_wal_keep_size by
> > using --new-options like '--new-options  "-c
> > max_slot_wal_keep_size=val"':
> > +       /*
> > +        * Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> > +        * checkpointer process.  If WALs required by logical replication slots
> > +        * are removed, the slots are unusable.  This setting prevents the
> > +        * invalidation of slots during the upgrade. We set this option when
> > +        * cluster is PG17 or later because logical replication slots
> > can only be
> > +        * migrated since then. Besides, max_slot_wal_keep_size is
> > added in PG13.
> > +        */
> > +       if (GET_MAJOR_VERSION(cluster->major_version) >= 1700)
> > +               appendPQExpBufferStr(&pgoptions, " -c
> > max_slot_wal_keep_size=-1");
> >
> > Should there be a check to throw an error if this option is specified
> > or do we need some documentation that this option should not be
> > specified?
>
> Hmm, I don't think we have to add checks. Other settings, like synchronous_commit
> and fsync, can be also overwritten, but pg_upgrade has never checked. Therefore,
> it's user's responsibility to not set max_slot_wal_keep_size to a dangerous
> value.
>
> > 2) Because we are able to override max_slot_wal_keep_size there is a
> > chance of slot getting invalidated and Assert being hit:
> > +               /*
> > +                * The logical replication slots shouldn't be invalidated as
> > +                * max_slot_wal_keep_size GUC is set to -1 during the
> > upgrade.
> > +                *
> > +                * The following is just a sanity check.
> > +                */
> > +               if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> > +               {
> > +                       Assert(max_slot_wal_keep_size_mb == -1);
> > +                       elog(ERROR, "replication slots must not be
> > invalidated during the upgrade");
> > +               }
>
> Hmm, so how about removing an assert and changing the error message more
> appropriate? I still think it seldom occurs.

As this scenario can occur by overriding max_slot_wal_keep_size, it is
better to remove the Assert.

Regards,
Vignesh