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