Re: Handle infinite recursion in logical replication setup

On Wed, Aug 31, 2022 at 11:35 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are my review comments for v43-0001.
>
> ======
>
> 1. Commit message
>
> The initial copy phase has no way to know the origin of the row data,
> so if 'copy_data = true' in the step 4 below, log a warning to notify user
> that potentially non-local data might have been copied.
>
> 1a.
> "in the step 4" -> "in step 4"
>
> ~
>
> 1b.
> "notify user" -> "notify the user"
>
> ======
>
> 2. doc/src/sgml/ref/create_subscription.sgml
>
> + <para>
> + If the subscription is created with <literal>origin = none</literal> and
> + <literal>copy_data = true</literal>, it will check if the publisher has
> + subscribed to the same table from other publishers and, if so, log a
> + warning to notify the user to check the publisher tables. Before continuing
> + with other operations the user should check that publisher tables did not
> + have data with different origins to prevent data inconsistency issues on the
> + subscriber.
> + </para>
>
> I am not sure about that wording saying "to prevent data inconsistency
> issues" because I thought maybe is already too late because any
> unwanted origin data is already copied during the initial sync. I
> think the user can do it try to clean up any problems caused before
> things become even worse (??)
>

Oh no, it is not too late in all cases. The problem can only occur if
the user will try to subscribe from all the different publications
with copy_data = true. We are anyway trying to clarify in the second
patch the right way to accomplish this. So, I am not sure what better
we can do here. The only bulletproof way is to provide some APIs where
users don't need to bother about all these cases, basically something
similar to what Kuroda-San is working on in the thread [1].

> ~~~
>
> You asked for my thoughts (see [1] 6b):
>
> > There is nothing much a user can do in this case. Only option would be
> > to take a backup before the operation and restore it and then recreate
> > the replication setup. I was not sure if we should document these
> > steps as similar inconsistent data could get created without the
> > origin option . Thoughts?
>
> I think those cases are a bit different:
>
> - For a normal scenario (i.e. origin=ANY) then inconsistent data just
> refers to problems like maybe PK violations so I think you just get
> what you get and have to deal with the errors...
>
> - But when the user said origin=NONE they are specifically saying they
> do NOT want any non-local data, yet they still might end up getting
> data contrary to their wishes. So I think maybe there ought to be
> something documented to warn about this potentially happening. My
> first impression is that all this seems like a kind of terrible
> problem because if it happens then cleanup could be difficult - if
> that subscriber in turn was also a publisher then this bad data might
> have propagated all over the place already! Anyway, I guess all this
> could be mentioned in some (new ?) section of the
> logical-replication.sgml file (i.e. patch 0002).
>
> IDEA: I was wondering if the documentation should recommend (or maybe
> we can even enforce this) that when using origin=NONE the user should
> always create the subscription with enabled=FALSE. That way if there
> are some warnings about potential bad origin data then there it might
> be possible to take some action or change their mind *before* any
> unwanted data pollutes everything.
>

If we want to give this suggestion, then we need to also say that this
is only valid when copy_data is true. The other drawback is if users
always need to do this to use origin=NONE then this can be a burden to
the user. I am not against having such a NOTE but the tone should not
be to enforce users to do this.

[1] - https://www.postgresql.org/message-id/CAHut%2BPuwRAoWY9pz%3DEubps3ooQCOBFiYPU9Yi%3DVB-U%2ByORU7OA%40mail.gmail.com

--
With Regards,
Amit Kapila.