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." > 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." > 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. -- With Regards, Amit Kapila.
