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

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.