Re: Handle infinite recursion in logical replication setup

On Mon, Aug 29, 2022 at 11:59 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch v42-0001:
>
> ~~
>
> 6.
>
> +   warning to notify user to check the publisher tables. The user can ensure
> +   that publisher tables do not have data which has an origin associated before
> +   continuing with any other operations to prevent inconsistent data being
> +   replicated.
> +  </para>
>
> 6a.
>
> I'm not so sure about this. IMO the warning is not about the
> replication – really it is about the COPY which has already happened
> anyway. So the user can't really prevent anything from going wrong;
> instead, they have to take some action to clean up if anything did go
> wrong.
>
> SUGGESTION
> Before continuing with other operations the user should check that
> publisher tables did not have data with different origins, otherwise
> inconsistent data may have been copied.
>
> ~

I would like to avoid the use of 'may' here. So, let's change it to
something like: "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."
>
> 6b.
>
> I am also wondering what can the user do now. Assuming there was bad
> COPY then the subscriber table has already got unwanted stuff copied
> into it. Is there any advice we can give to help users fix this mess?
>

I don't think the user can do much in that case. She will probably
need to re-create the replication.

>
> 11.
>
> + ereport(WARNING,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("publisher has subscribed table \"%s.%s\" from some other publisher",
> +    nspname, relname),
> + errdetail("Publisher might have subscribed one or more tables from
> some other publisher."),
> + errhint("Verify that these publisher tables do not have data that
> has an origin associated before proceeding to avoid inconsistency."));
> + break;
>
> 11a.
> I'm not sure having that "break" code is correct logic. Won't that
> mean the user will only get a warning for the first potential problem
> encountered but then other potential problems tables will have no
> warnings at all so the user may not be made aware of them? Perhaps you
> need to first gather the list of all the suspicious tables before
> logging a single warning which shows that list? Or perhaps you need to
> log multiple warnings – one warning per suspicious table?
>
> ~
>
> 11b.
> The WARNING message seems a bit inside-out because the errmsg is
> giving more details than the errdetail.
>
> SUGGESTIONS (or something similar)
> errmsg - "subscription XXX requested origin=NONE but may have copied
> data that had a different origin."
> errdetail – Publisher YYY has subscribed table \"%s.%s\" from some
> other publisher"
>

Your suggestion is better but we can't say 'copied' because the copy
may start afterwards by tablesync worker. Also, using 'may' is not
advisable in error messages. How about : "subscription XXX requested
origin=NONE but might copy data that had a different origin."?

-- 
With Regards,
Amit Kapila.