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

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.

> 3) File 003_logical_replication_slots.pl is now changed to
> 003_upgrade_logical_replication_slots.pl, it should be change here too
> accordingly:
> index 5834513add..815d1a7ca1 100644
> --- a/src/bin/pg_upgrade/Makefile
> +++ b/src/bin/pg_upgrade/Makefile
> @@ -3,6 +3,9 @@
>  PGFILEDESC = "pg_upgrade - an in-place binary upgrade utility"
>  PGAPPICON = win32
> 
> +# required for 003_logical_replication_slots.pl
> +EXTRA_INSTALL=contrib/test_decoding
> +

Fixed.

[1]: https://www.postgresql.org/message-id/TYCPR01MB587007EA2F9AB92F0E1F5957F5D4A%40TYCPR01MB5870.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED