On Mon, Sep 11, 2023 at 11:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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."
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