On Mon, Sep 11, 2023 at 11:16 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Sep 11, 2023 at 10:39 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > 3. > > - with the primary.) Replication slots are not copied and must > > - be recreated. > > + with the primary.) Replication slots on the old standby are not > > copied. > > + Only logical slots on the primary are migrated to the new standby, > > + and other slots must be recreated. > > > > This paragraph should be rephrased. I mean first stating that > > "Replication slots on the old standby are not copied" and then saying > > Only logical slots are migrated doesn't seem like the best way. Maybe > > we can just say "Only logical slots on the primary are migrated to the > > new standby, and other slots must be recreated." > > > > It is fine to combine these sentences but let's be a bit more > explicit: "Only logical slots on the primary are migrated to the new > standby, and other slots on the old standby must be recreated as they > are not copied."
Fine with this. > > 4. > > + /* > > + * Raise an ERROR if the logical replication slot is invalidating. It > > + * would not happen because max_slot_wal_keep_size is set to -1 during > > + * the upgrade, but it stays safe. > > + */ > > + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) > > + elog(ERROR, "Replication slots must not be invalidated during the > > upgrade."); > > > > Rephrase the first line as -> Raise an ERROR if the logical > > replication slot is invalidating during an upgrade. > > > > I think it would be better to write something like: "The logical > replication slots shouldn't be invalidated as max_slot_wal_keep_size > is set to -1 during the upgrade." This makes it much clear. > > 5. > > + /* Logical slots can be migrated since PG17. */ > > + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600) > > + return; > > > > > > For readability change this to if > > (GET_MAJOR_VERSION(old_cluster.major_version) < 1700), because in most > > of the checks related to this, we are using 1700 so better be > > consistent in this. > > > > But the current check is consistent with what we do at other places > during the upgrade. I think the patch is trying to be consistent with > existing code as much as possible. Okay, I see. Thanks for pointing that out. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
