public inbox for [email protected]
help / color / mirror / Atom feedRe: Handle infinite recursion in logical replication setup
16+ messages / 5 participants
[nested] [flat]
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 05:20 vignesh C <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: vignesh C @ 2022-03-07 05:20 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 06:14 Peter Smith <[email protected]>
parent: vignesh C <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Peter Smith @ 2022-03-07 06:14 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 4:20 PM vignesh C <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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.
>
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 07:17 vignesh C <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 3 replies; 16+ messages in thread
From: vignesh C @ 2022-03-07 07:17 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 11:45 AM Peter Smith <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 4:20 PM vignesh C <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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.
> >
>
> 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?
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%40ma...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 16+ messages in thread
* RE: Handle infinite recursion in logical replication setup
@ 2022-03-07 07:37 [email protected] <[email protected]>
parent: vignesh C <[email protected]>
2 siblings, 0 replies; 16+ messages in thread
From: [email protected] @ 2022-03-07 07:37 UTC (permalink / raw)
To: 'vignesh C' <[email protected]>; Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 08:14 Peter Smith <[email protected]>
parent: vignesh C <[email protected]>
2 siblings, 1 reply; 16+ messages in thread
From: Peter Smith @ 2022-03-07 08:14 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 6:17 PM vignesh C <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 11:45 AM Peter Smith <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 4:20 PM vignesh C <[email protected]> wrote:
> > >
> > > On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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.
> > >
> >
> > 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?
>
> 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%40ma...
>
> Regards,
> Vignesh
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 08:54 Amit Kapila <[email protected]>
parent: vignesh C <[email protected]>
2 siblings, 0 replies; 16+ messages in thread
From: Amit Kapila @ 2022-03-07 08:54 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 12:48 PM vignesh C <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 11:45 AM Peter Smith <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 4:20 PM vignesh C <[email protected]> wrote:
> > >
> > > On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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.
> > >
> >
> > 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?
>
> 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?
>
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.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 10:53 vignesh C <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: vignesh C @ 2022-03-07 10:53 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 1:45 PM Peter Smith <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 6:17 PM vignesh C <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 11:45 AM Peter Smith <[email protected]> wrote:
> > >
> > > On Mon, Mar 7, 2022 at 4:20 PM vignesh C <[email protected]> wrote:
> > > >
> > > > On Mon, Mar 7, 2022 at 10:26 AM Peter Smith <[email protected]> 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.
> > > >
> > >
> > > 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?
> >
> > 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%40ma...
> >
> > Regards,
> > Vignesh
>
> 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;
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.gma...
[2] - https://www.postgresql.org/message-id/CAFiTN-tKbjHDjAFNnqRoR8u1B%2Bfs0wunGz%3D3wp0iU-sUaxZJTQ%40mail...
[3] - https://www.postgresql.org/message-id/CAA4eK1%2Bco2cd8a6okgUD_pcFEHcc7mVc0k_RE2%3D6ahyv3WPRMg%40mail...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 11:30 Ashutosh Bapat <[email protected]>
parent: vignesh C <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Ashutosh Bapat @ 2022-03-07 11:30 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Peter Smith <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 12:20 Amit Kapila <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Amit Kapila @ 2022-03-07 12:20 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: vignesh C <[email protected]>; Peter Smith <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 5:01 PM Ashutosh Bapat
<[email protected]> wrote:
>
> 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 thought it should be similar to 'streaming' option of subscription
but may be Vignesh has some other reason which makes it different.
> 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.
>
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.
> 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.
>
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...
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-07 16:27 vignesh C <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 2 replies; 16+ messages in thread
From: vignesh C @ 2022-03-07 16:27 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Peter Smith <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 5:51 PM Amit Kapila <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 5:01 PM Ashutosh Bapat
> <[email protected]> wrote:
> >
> > 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 thought it should be similar to 'streaming' option of subscription
> 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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-08 05:00 Peter Smith <[email protected]>
parent: vignesh C <[email protected]>
1 sibling, 1 reply; 16+ messages in thread
From: Peter Smith @ 2022-03-08 05:00 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Amit Kapila <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-08 05:21 Amit Kapila <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Amit Kapila @ 2022-03-08 05:21 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: vignesh C <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Mar 8, 2022 at 10:31 AM Peter Smith <[email protected]> wrote:
>
> 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)
>
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.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-08 06:46 Peter Smith <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Peter Smith @ 2022-03-08 06:46 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: vignesh C <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Mar 8, 2022 at 4:21 PM Amit Kapila <[email protected]> wrote:
>
> On Tue, Mar 8, 2022 at 10:31 AM Peter Smith <[email protected]> wrote:
> >
> > 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)
> >
>
> 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?
>
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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-08 07:54 Amit Kapila <[email protected]>
parent: Peter Smith <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Amit Kapila @ 2022-03-08 07:54 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: vignesh C <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, Mar 8, 2022 at 12:17 PM Peter Smith <[email protected]> wrote:
>
> On Tue, Mar 8, 2022 at 4:21 PM Amit Kapila <[email protected]> wrote:
> >
> > On Tue, Mar 8, 2022 at 10:31 AM Peter Smith <[email protected]> wrote:
> > >
> > > 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)
> > >
> >
> > 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?
> >
>
> 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?
>
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.
> 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?
>
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.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Handle infinite recursion in logical replication setup
@ 2022-03-08 14:46 vignesh C <[email protected]>
parent: vignesh C <[email protected]>
1 sibling, 0 replies; 16+ messages in thread
From: vignesh C @ 2022-03-08 14:46 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Peter Smith <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 7, 2022 at 9:57 PM vignesh C <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 5:51 PM Amit Kapila <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 5:01 PM Ashutosh Bapat
> > <[email protected]> wrote:
> > >
> > > 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 thought it should be similar to 'streaming' option of subscription
> > 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.
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%...
Regards,
Vignesh
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Include sequences in publications created by pg_createsubscriber
@ 2026-07-09 23:05 Peter Smith <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Peter Smith @ 2026-07-09 23:05 UTC (permalink / raw)
To: vignesh C <[email protected]>; +Cc: Shlok Kyal <[email protected]>; PostgreSQL Hackers <[email protected]>
v9 LGTM.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-07-09 23:05 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 05:20 Re: Handle infinite recursion in logical replication setup vignesh C <[email protected]>
2022-03-07 06:14 ` Peter Smith <[email protected]>
2022-03-07 07:17 ` vignesh C <[email protected]>
2022-03-07 07:37 ` [email protected] <[email protected]>
2022-03-07 08:14 ` Peter Smith <[email protected]>
2022-03-07 10:53 ` vignesh C <[email protected]>
2022-03-07 11:30 ` Ashutosh Bapat <[email protected]>
2022-03-07 12:20 ` Amit Kapila <[email protected]>
2022-03-07 16:27 ` vignesh C <[email protected]>
2022-03-08 05:00 ` Peter Smith <[email protected]>
2022-03-08 05:21 ` Amit Kapila <[email protected]>
2022-03-08 06:46 ` Peter Smith <[email protected]>
2022-03-08 07:54 ` Amit Kapila <[email protected]>
2022-03-08 14:46 ` vignesh C <[email protected]>
2022-03-07 08:54 ` Amit Kapila <[email protected]>
2026-07-09 23:05 Re: Include sequences in publications created by pg_createsubscriber Peter Smith <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox