On Thu, Jan 11, 2024 at 9:08 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 10, 2024 at 06:02:12PM +0530, Amit Kapila wrote:
> > - 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.
>
> As far as I understand your complaint is about being more precise
> about where the workers could run when we do an upgrade. My patch
> covers the reason why it would be a problem, and I agree that it could
> be more detailed.
>
> Hence, how about something like that:
> "The logical replication launcher is disabled during binary upgrades,
> as a logical replication workers running on the cluster upgrading from
> could cause replication origins to move forward after they are copied
> to the cluster upgrading to, creating potentially conflicts with the
> physical files copied over."
>
Looks better. One minor nitpick: /potentially/potential
--
With Regards,
Amit Kapila.