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."
If you have a better suggestion, feel free.
--
Michael