Subject: Re: Handle infinite recursion in logical replication setup

From: vignesh C <[email-masked]>
Date: Mon, 7 Mar 2022 10:50:35 +0530

On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email-masked]> wrote:
>
> Hi Vignesh, I also have not looked at the patch yet, but I have what
> seems like a very fundamental (and possibly dumb) question...
>
> Basically, I do not understand the choice of syntax for setting things up.
>
> IMO that "only-local" option sounds very similar to the other
> PUBLICATION ("publish") options which decide the kinds of things that
> will be published. So it feels more natural for me to think of the
> publisher as being the one to decide what will be published.
>
> e.g.
>
> option 1:
> CREATE PUBLICATION p1 FOR TABLE t1;
> CREATE SUBSCRITION s1 ... FOR PUBLICATION p1 WITH (only_local = true);
>
> option 2:
> CREATE PUBLICATION p1 FOR TABLE t1 WEHRE (publish = 'only_local');
> CREATE SUBSCRITION s1 ... FOR PUBLICATION p1;
>
> ~~
>
> IIUC the patch is using option 1. My first impression was it feels
> back-to-front for the SUBSCRIPTION telling the PUBLICATION what to
> publish.
>
> So, why does the patch use syntax option 1?

I felt the advantage with keeping it at the subscription side is that,
the subscriber from one node can subscribe with only_local option on
and a different subscriber from a different node can subscribe with
only_local option as off. This might not be possible with having the
option at publisher side. Having it at the subscriber side might give
more flexibility for the user.

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Mon, 7 Mar 2022 17:14:48 +1100

> more flexibility for the user.

OK.  Option 2 needs two publications for that scenario. IMO it's more
intuitive this way, but maybe you wanted to avoid the extra
publications?

node0:
CREATE PUBLICATION p1 FOR TABLE t1;
CREATE PUBLICATION p1_local FOR TABLE t1 WITH (publish = 'only_local');

node1: CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1_local;
node2: CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1;

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: vignesh C <[email-masked]>
Date: Mon, 7 Mar 2022 12:47:43 +0530

> intuitive this way, but maybe you wanted to avoid the extra

Yes, I wanted to avoid the extra publication creation that you pointed
out. Option 1 can handle this scenario without creating the extra
publications:
node0: CREATE PUBLICATION p1 FOR TABLE t1;
node1: CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1 with (only_local = on);
node2:  CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1 with (only_local = off);

I'm ok with both the approaches, now that this scenario can be handled
by using both the options. i.e providing only_local option as an
option while creating publication or providing only_local option as an
option while creating subscription as Peter has pointed out at [1].
option 1:
CREATE PUBLICATION p1 FOR TABLE t1;
CREATE SUBSCRITION s1 ... FOR PUBLICATION p1 WITH (only_local = true);

option 2:
CREATE PUBLICATION p1 FOR TABLE t1 WITH (publish = 'only_local');
CREATE SUBSCRITION s1 ... FOR PUBLICATION p1;

Shall we get a few opinions on this and take it in that direction?

[1] - https://www.postgresql.org/message-id/CAHut%2BPsAWaETh9VMymbBfMrqiE1KuqMq%2BwpBg0s7eMzwLATr%2Bw%40mail.gmail.com

Regards,
Vignesh

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 7 Mar 2022 07:37:46 +0000

Dear Vignesh,

> I felt changing only_local option might be useful for the user while
> modifying the subscription like setting it with a different set of
> publications. Changes for this are included in the v2 patch attached
> at [1].

+1, thanks. I'll post if I notice something to say.

> Shall we get a few opinions on this and take it in that direction?

I prefer subscriber-option, but I also think both are reasonable.
+1 about asking other reviewers.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Smith <[email-masked]>
Date: Mon, 7 Mar 2022 19:14:39 +1100

> [1] - https://www.postgresql.org/message-id/CAHut%2BPsAWaETh9VMymbBfMrqiE1KuqMq%2BwpBg0s7eMzwLATr%2Bw%40mail.gmail.com

BTW here is a counter-example to your scenario from earlier.

Let's say I have a publication p1 and p2 and want to subscribe to p1
with only_local=true, and p2 with only_local = false;

Using the current OPtion 1 syntax you cannot do this with a single
subscription because the option is tied to the subscription.
But using syntax Option 2 you may be able to do it.

Option 1:
CREATE PUBLICATION p1 FOR TABLE t1;
CREATE PUBLICATION p2 FOR TABLE t2;
CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1 WITH (local_only = true);
CREATE SUBSCRIPTION s2 ... FOR PUBLICATION p1 WITH (local_only = false);

Option 2:
CREATE PUBLICATION p1 FOR TABLE t1 WITH (publish = 'local_only');
CREATE PUBLICATION p2 FOR TABLE t2;
CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1, p2;

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 7 Mar 2022 14:24:43 +0530

> Shall we get a few opinions on this and take it in that direction?

Allowing multiple publications will lead to a lot of duplicate data in
catalogs like pg_publication_rel, so, I don't see how option-2 can be
beneficial?

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Mon, 7 Mar 2022 16:23:17 +0530

> CREATE SUBSCRIPTION s1 ... FOR PUBLICATION p1, p2;

I felt having multiple publications will create duplicate entries in
the system table, Amit also has pointed this at [1]. Also enhancing
this approach to support filtering based on replication origin which
is suggested by dilip at [2] is also on the client side and also the
initial check to handle the copy_data specified by Amit at [3] will be
done by the client side. Based on the above I feel the existing
approach is better. I might be missing something here.

[1] - https://www.postgresql.org/message-id/CAA4eK1LgCVv8u-fOsMPbGC96sWXhT3EKOBAeFW3g84otjStztw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAFiTN-tKbjHDjAFNnqRoR8u1B%2Bfs0wunGz%3D3wp0iU-sUaxZJTQ%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAA4eK1%2Bco2cd8a6okgUD_pcFEHcc7mVc0k_RE2%3D6ahyv3WPRMg%40mail.gmail.com

Regards,
Vignesh

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 7 Mar 2022 17:00:56 +0530

Hi Vignesh,
I agree with Peter's comment that the changes to
FilterRemoteOriginData() should be part of FilterByOrigin()

Further, I wonder why "onlylocal_data" is a replication slot's
property. A replication slot tracks the progress of replication and it
may be used by different receivers with different options. I could
start one receiver which wants only local data, say using
"pg_logical_slot_get_changes" and later start another receiver which
fetches all the data starting from where the first receiver left. This
option prevents such flexibility.

As discussed earlier in the thread, local_only can be property of
publication or subscription, depending upon the use case, but I can't
see any reason that it should be tied to a replication slot.

I have a similar question for "two_phase" but the ship has sailed and
probably it makes some sense there which I don't know.

As for publication vs subscription, I think both are useful cases.
1. It will be a publication's property, if we want the node to not
publish any data that it receives from other nodes for a given set of
tables.
2. It will be the subscription's property, if we want the subscription
to decide whether it wants to fetch the data changed on only upstream
or other nodes as well.

Maybe we want to add it to both and use the stricter of both. If a
publication has local_only, it publishes only local changes. If the
subscription has local only then WAL sender sends only local changes.

--
Best Wishes,
Ashutosh

---

From: Amit Kapila <[email-masked]>
Date: Mon, 7 Mar 2022 17:50:59 +0530

> see any reason that it should be tied to a replication slot.

I thought it should be similar to 'streaming' option of subscription
but may be Vignesh has some other reason which makes it different.

> probably it makes some sense there which I don't know.

two_phase is different from some of the other subscription options
like 'streaming' such that it can be enabled only at the time of slot
and subscription creation, we can't change/specify it via
pg_logical_slot_get_changes. This is to avoid the case where we won't
know at the time of the commit prepared whether the prepare for the
transaction has already been sent. For the same reason, we need to
also know the 'two_phase_at' information.

> to decide whether it wants to fetch the data changed on only upstream

I think it could be useful to allow it via both publication and
subscription but I guess it is better to provide it via one way
initially just to keep things simple and give users some way to deal
with such cases. I would prefer to allow it via subscription initially
for the reasons specified by Vignesh in his previous email [1]. Now,
if we think those all are ignorable things and it is more important to
allow this option first by publication or we must allow it via both
publication and subscription then it makes sense to change it.

[1] - https://www.postgresql.org/message-id/CALDaNm3jkotRhKfCqu5CXOf36_yiiW_cYE5%3DbG%3Dj6N3gOWJkqw%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Mon, 7 Mar 2022 21:57:03 +0530

> but may be Vignesh has some other reason which makes it different.

Yes, this can be removed from the replication slot. It is my mistake
that I have made while making the code similar to two-phase, I'm
working on making the changes for this. I will fix and post an updated
version for this.

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Tue, 8 Mar 2022 16:00:45 +1100

IIUC the new option may be implemented subscriber-side and/or
publisher-side and/or both, and the subscriber-side option may be
"enhanced" in future to prevent cycles. And probably there are more
features I don't know about or that have not yet been thought of.

~~

Even if the plan is only to implement just one part now and then add
more later, I think there still should be some consideration for what
you expect all possible future options to look like, because that may
affect current implementation choices.

The point is:

- we should take care so don't accidentally end up with an option that
turned out to be inconsistent looking on the subscriber-side /
publisher-side.

- we should try to avoid accidentally painting ourselves into a corner
(e.g. stuck with a boolean option that cannot be enhanced later on)

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Tue, 8 Mar 2022 10:51:08 +0530

> (e.g. stuck with a boolean option that cannot be enhanced later on)

Agreed. I think it is important to see we shouldn't do something which
is not extendable in future or paint us in the corner. But, as of now,
with the current proposal, the main thing we should consider is
whether exposing the boolean option is okay or shall we do something
different so that we can extend it later to specific origin ids?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Tue, 8 Mar 2022 17:46:37 +1100

> different so that we can extend it later to specific origin ids?

I was wondering, assuming later there is an enhancement to detect and
prevent cycles using origin ids, then what really is the purpose of
the option?

I can imagine if a cycle happens should probably log some one-time
WARNING (just to bring it to the user's attention in case it was
caused by some accidental misconfiguration) but apart from that why
would this need to be an option at all - i.e. is there a use-case
where a user would NOT want to prevent a recursive error?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Tue, 8 Mar 2022 13:24:47 +0530

> prevent cycles using origin ids, then what really is the purpose of

We can't use origin ids during the initial sync as we can't
differentiate between data from local or remote node. Also, how will
we distinguish between different origins? AFAIU, all the changes
applied on any particular node use the same origin.

> where a user would NOT want to prevent a recursive error?

I think there will be plenty of such setups where there won't be a
need for any such option like where users won't need bi-directional
replication for the same table.

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Tue, 8 Mar 2022 20:16:24 +0530

> working on making the changes for this. I will fix and post an updated

I have made the changes for this, the changes for this are available
in the v3 patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0JcV-7iQZhyy3kehnWTy6x%3Dz%2BsX6u6Df%2B%2By8z33pz%2BBw%40mail.gmail.com

Regards,
Vignesh