Re: Handle infinite recursion in logical replication setup

On Thu, Aug 4, 2022 at 6:31 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Aug 2, 2022 at 8:59 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jul 26, 2022 at 9:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > Let me try to summarize the discussion so that it is easier for others
> > to follow. The work in this thread is to avoid loops, and duplicate
> > data in logical replication when the operations happened on the same
> > table in multiple nodes. It has been explained in email [1] with an
> > example of how a logical replication setup can lead to duplicate or
> > inconsistent data.
> >
> > The idea to solve this problem is that we don't replicate data that is
> > not generated locally which we can normally identify based on origin
> > of data in WAL. The commit 366283961a achieves that for replication
> > but still the problem can happen during initial sync which is
> > performed internally via copy. We can't differentiate the data in heap
> > based on origin. So, we decided to prohibit the subscription
> > operations that can perform initial sync (ex. Create Subscription,
> > Alter Subscription ... Refresh) by detecting that the publisher has
> > subscribed to the same table from some other publisher.
> >
> > To prohibit the subscription operations, the currently proposed patch
> > throws an error. Then, it also provides a new copy_data option
> > 'force' under which the user will still be able to perform the
> > operation. This could be useful when the user intentionally wants to
> > replicate the initial data even if it contains data from multiple
> > nodes (for example, when in a multi-node setup, one decides to get the
> > initial data from just one node and then allow replication of data to
> > proceed from each of respective nodes).
> >
> > The other alternative discussed was to just give a warning for
> > subscription operations and probably document the steps for users to
> > avoid it. But the problem with that is once the user sees this
> > warning, it won't be able to do anything except recreate the setup, so
> > why not give an error in the first place?
> >
> > Thoughts?
>
> Thank you for the summary!
>
> I understand that this feature could help some cases, but I'm really
> not sure adding new value 'force' for them is worthwhile, and
> concerned it could reduce the usability.
>
> IIUC this feature would work only when origin = 'none' and copy_data =
> 'on', and the purpose is to prevent the data from being
> duplicated/conflicted by the initial table sync. But there are cases
> where duplication/conflict doesn't happen even if origin = 'none' and
> copy_data = 'on'. For instance, the table on the publisher might be
> empty.
>

Right, but if we want we can check the tables on publishers to ensure
that. Now, another case could be where the corresponding subscription
was disabled on publisher during create subscription but got enabled
just before copy, we can even catch that situation as we are doing for
column lists in fetch_remote_table_info(). Are there other cases of
false positives you can think of? I see your point that we can
document that users should be careful with certain configurations to
avoid data inconsistency but not able completely convince myself about
the same. I think the main thing to decide here is how much we want to
ask users to be careful by referring them to docs. Now, if you are not
convinced with giving an ERROR here then we can probably show a
WARNING (that we might copy data for multiple origins during initial
sync in spite of the user having specified origin as NONE)?

>
Also, even with origin = 'any', copy_data = 'on', there is a
> possibility of data duplication/conflict. Why do we need to address
> only the case where origin = 'none'?
>

Because the user has specifically asked not to replicate any remote
data by specifying origin = NONE, which should be dealt with
differently whereas 'any' doesn't have such a requirement. Now,
tomorrow, if we want to support replication based on specific origin
names say origin = 'node-1' then also we won't be able to identify the
data during initial sync but I think 'none' is a special case where
giving some intimation to user won't be a bad idea especially because
we can identify the same.

> I think that using origin =
> 'none' doesn't necessarily mean using bi-directional (or N-way)
> replication. Even when using uni-directional logical replication with
> two nodes, they may use origin = 'none'.
>

It is possible but still, I think it is a must for bi-directional (or
N-way) replication, otherwise, there is a risk of loops.

--
With Regards,
Amit Kapila.