On Wed, Jan 10, 2024 at 10:11 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 15, 2023 at 07:58:06AM +0530, Amit Kapila wrote:
> > I am fine with this but there is no harm in doing this before or along
> > with the main patch. As of now, I don't see any problem but as the
> > main patch is still under review, so thought we could even wait for
> > the patch to become "Ready For Committer".
>
> My apologies for the delay.
>
> Now that 9a17be1e244a is in the tree, please find attached a patch to
> restrict the startup of the launcher using IsBinaryUpgrade in
> ApplyLauncherRegister(), with adjustments to the surrounding comments.
>
- if (max_logical_replication_workers == 0)
+ /*
+ * The logical replication launcher is disabled during binary upgrades,
+ * as logical replication workers can stream data during the upgrade
+ * which can cause replication origins to move forward after we have
+ * copied them. It can cause the system to request the data which is
+ * already present in the new cluster.
+ */
+ if (max_logical_replication_workers == 0 || IsBinaryUpgrade)
This comment is not very clear to me. The first part of the sentence
can't apply to the new cluster as after the upgrade, subscriptions
will be disabled and the second part talks about requesting the wrong
data in the new cluster. As per my understanding, the problem here is
that, on the old cluster, the origins may move forward after we copy
them and then we copy physical files. Now, in the new cluster when we
try to request the data, it will be already present.
> Was there anything else you wanted to be covered and/or updated?
>
No, only this patch.
--
With Regards,
Amit Kapila.
