On Thu, Jan 11, 2024 at 9:08 AM Michael Paquier <mich...@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.
