On Mon, Sep 11, 2023 at 10:39 AM Dilip Kumar <dilipbalaut@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.