Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

On Tue, 26 Jan 2021 at 11:47, japin <japinli@hotmail.com> wrote:

Hi, hackers

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

[1] - /messages/by-id/MEYP282MB16690CD5EC5319FC35B2F78AB6BD0@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

Sorry, I forgot to attach the patch.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Tue, Jan 26, 2021 at 9:25 AM japin <japinli@hotmail.com> wrote:

I think it's more convenient. Any thoughts?

Sorry, I forgot to attach the patch.

As I mentioned earlier in [1]/messages/by-id/CALj2ACVGDNZDQk3wfv=3zYTg=qKUaEa5s1f+9_PYxN0QyAUdCw@mail.gmail.com, +1 from my end to have the new syntax
for adding/dropping publications from subscriptions i.e. ALTER
SUBSCRIPTION ... ADD/DROP PUBLICATION. But I'm really not sure why
that syntax was not added earlier. Are we missing something here?

I would like to hear opinions from other hackers.

[1]: /messages/by-id/CALj2ACVGDNZDQk3wfv=3zYTg=qKUaEa5s1f+9_PYxN0QyAUdCw@mail.gmail.com

Some quick comments on the patch, although I have not taken a deeper look at it:

1. IMO, it will be good if the patch is divided into say coding, test
cases and documentation
2. Looks like AlterSubscription() is already having ~200 LOC, why
can't we have separate functions for each ALTER_SUBSCRIPTION_XXXX case
or at least for the new code that's getting added for this patch?
3. The new code added for ALTER_SUBSCRIPTION_ADD_PUBLICATION and
ALTER_SUBSCRIPTION_DROP_PUBLICATION looks almost same maybe with
little difference, so why can't we have single function
(alter_subscription_add_or_drop_publication or
hanlde_add_or_drop_publication or some better name?) and pass in a
flag to differentiate the code that differs for both commands.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Hi, Bharath

Thanks for your reviewing.

On Tue, 26 Jan 2021 at 12:55, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:25 AM japin <japinli@hotmail.com> wrote:

I think it's more convenient. Any thoughts?

Sorry, I forgot to attach the patch.

As I mentioned earlier in [1], +1 from my end to have the new syntax
for adding/dropping publications from subscriptions i.e. ALTER
SUBSCRIPTION ... ADD/DROP PUBLICATION. But I'm really not sure why
that syntax was not added earlier. Are we missing something here?

Yeah, we should figure out why we do not support this syntax earlier. It seems
ALTER SUBSCRIPTION is introduced in 665d1fad99e, however the commit do not
contains any discussion URL.

I would like to hear opinions from other hackers.

[1] - /messages/by-id/CALj2ACVGDNZDQk3wfv=3zYTg=qKUaEa5s1f+9_PYxN0QyAUdCw@mail.gmail.com

Some quick comments on the patch, although I have not taken a deeper look at it:

1. IMO, it will be good if the patch is divided into say coding, test
cases and documentation

Agreed. I will refactor it in next patch.

2. Looks like AlterSubscription() is already having ~200 LOC, why
can't we have separate functions for each ALTER_SUBSCRIPTION_XXXX case
or at least for the new code that's getting added for this patch?

I'm not sure it is necessary to provide a separate functions for each
ALTER_SUBSCRIPTION_XXX, so I just followed current style.

3. The new code added for ALTER_SUBSCRIPTION_ADD_PUBLICATION and
ALTER_SUBSCRIPTION_DROP_PUBLICATION looks almost same maybe with
little difference, so why can't we have single function
(alter_subscription_add_or_drop_publication or
hanlde_add_or_drop_publication or some better name?) and pass in a
flag to differentiate the code that differs for both commands.

Agreed.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Tue, 26 Jan 2021 at 13:46, japin <japinli@hotmail.com> wrote:

Hi, Bharath

Thanks for your reviewing.

On Tue, 26 Jan 2021 at 12:55, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:25 AM japin <japinli@hotmail.com> wrote:

I think it's more convenient. Any thoughts?

Sorry, I forgot to attach the patch.

As I mentioned earlier in [1], +1 from my end to have the new syntax
for adding/dropping publications from subscriptions i.e. ALTER
SUBSCRIPTION ... ADD/DROP PUBLICATION. But I'm really not sure why
that syntax was not added earlier. Are we missing something here?

Yeah, we should figure out why we do not support this syntax earlier. It seems
ALTER SUBSCRIPTION is introduced in 665d1fad99e, however the commit do not
contains any discussion URL.

I would like to hear opinions from other hackers.

[1] - /messages/by-id/CALj2ACVGDNZDQk3wfv=3zYTg=qKUaEa5s1f+9_PYxN0QyAUdCw@mail.gmail.com

Some quick comments on the patch, although I have not taken a deeper look at it:

1. IMO, it will be good if the patch is divided into say coding, test
cases and documentation

Agreed. I will refactor it in next patch.

Split v1 path into coding, test cases, documentation and tab-complete.

2. Looks like AlterSubscription() is already having ~200 LOC, why
can't we have separate functions for each ALTER_SUBSCRIPTION_XXXX case
or at least for the new code that's getting added for this patch?

I'm not sure it is necessary to provide a separate functions for each
ALTER_SUBSCRIPTION_XXX, so I just followed current style.

3. The new code added for ALTER_SUBSCRIPTION_ADD_PUBLICATION and
ALTER_SUBSCRIPTION_DROP_PUBLICATION looks almost same maybe with
little difference, so why can't we have single function
(alter_subscription_add_or_drop_publication or
hanlde_add_or_drop_publication or some better name?) and pass in a
flag to differentiate the code that differs for both commands.

Agreed.

At present, I create a new function merge_subpublications() to merge the origin
publications and add/drop publications. Thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

Hi, hackers

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

+1 for the idea

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

Having said that, I don't deny the appeal of having separate commands
for each of (a), (b), and (c).

--
With Regards,
Amit Kapila.

On Wed, Jan 27, 2021 at 2:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

IIUC the initial patch proposed here, it does allow ALTER SUBSCRIPTION
mysub1 ADD/DROP PUBLICATION mypub4 WITH (refresh = true);. Isn't this
option enough to achieve what we can with ALTER SUBSCRIPTION mysub1
SET PUBLICATION mypub1, mypub2 WITH (refresh = true);? Am I missing
something here?

Having said that, I don't deny the appeal of having separate commands
for each of (a), (b), and (c).

for (c) i.e. refresh existing publication do we need something like
ALTER SUBSCRIPTION mysub1 REFRESH SUBSCRIPTION or some other syntax
that only refreshes the subscription similar to ALTER SUBSCRIPTION
mysub1 REFRESH PUBLICATION?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jan 27, 2021 at 2:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 2:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

IIUC the initial patch proposed here, it does allow ALTER SUBSCRIPTION
mysub1 ADD/DROP PUBLICATION mypub4 WITH (refresh = true);. Isn't this
option enough to achieve what we can with ALTER SUBSCRIPTION mysub1
SET PUBLICATION mypub1, mypub2 WITH (refresh = true);? Am I missing
something here?

I feel the SET syntax would allow refreshing existing publications as
well whereas, in Add, it will be only for new Publications.

--
With Regards,
Amit Kapila.

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

IIUC the initial patch proposed here, it does allow ALTER SUBSCRIPTION
mysub1 ADD/DROP PUBLICATION mypub4 WITH (refresh = true);. Isn't this
option enough to achieve what we can with ALTER SUBSCRIPTION mysub1
SET PUBLICATION mypub1, mypub2 WITH (refresh = true);? Am I missing
something here?

I feel the SET syntax would allow refreshing existing publications as
well whereas, in Add, it will be only for new Publications.

I think the patch v2-0001 will refresh all the publications, I mean
existing and newly added publications. IIUC the patch, it first
fetches all the available publications with the subscriptions and it
sees if that list has the given publication [1]+ +/* + * merge current subpublications and user specified by add/drop publications. + * + * If addpub == true, we will add the list of publications into current + * subpublications. Otherwise, we will delete the list of publications from + * current subpublications. + */ +static List * +merge_subpublications(HeapTuple tuple, TupleDesc tupledesc, + List *publications, bool addpub), if not, then adds it
to the existing publications list and returns that list [2]+ publications = merge_subpublications(tup, RelationGetDescr(rel),. If the
refresh option is specified as true with ALTER SUBSCRIPTION ... ADD
PUBLICATION, then it refreshes all the returned publications [3]+ /* Refresh if user asked us to. */ + if (refresh) + { + if (!sub->enabled) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"), + errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)."))); + + /* Make sure refresh sees the new list of publications. */ + sub->publications = publications; + + AlterSubscription_refresh(sub, copy_data);. I
believe this is also true with ALTER SUBSCRIPTION ... DROP
PUBLICATION.

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

[1]
+
+/*
+ * merge current subpublications and user specified by add/drop publications.
+ *
+ * If addpub == true, we will add the list of publications into current
+ * subpublications.  Otherwise, we will delete the list of publications from
+ * current subpublications.
+ */
+static List *
+merge_subpublications(HeapTuple tuple, TupleDesc tupledesc,
+                      List *publications, bool addpub)

[2]: + publications = merge_subpublications(tup, RelationGetDescr(rel),
+ publications = merge_subpublications(tup,
RelationGetDescr(rel),

[3]
+                /* Refresh if user asked us to. */
+                if (refresh)
+                {
+                    if (!sub->enabled)
+                        ereport(ERROR,
+                                (errcode(ERRCODE_SYNTAX_ERROR),
+                                 errmsg("ALTER SUBSCRIPTION with
refresh is not allowed for disabled subscriptions"),
+                                 errhint("Use ALTER SUBSCRIPTION ...
SET PUBLICATION ... WITH (refresh = false).")));
+
+                    /* Make sure refresh sees the new list of publications. */
+                    sub->publications = publications;
+
+                    AlterSubscription_refresh(sub, copy_data);

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, 27 Jan 2021 at 17:46, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

IIUC the initial patch proposed here, it does allow ALTER SUBSCRIPTION
mysub1 ADD/DROP PUBLICATION mypub4 WITH (refresh = true);. Isn't this
option enough to achieve what we can with ALTER SUBSCRIPTION mysub1
SET PUBLICATION mypub1, mypub2 WITH (refresh = true);? Am I missing
something here?

I feel the SET syntax would allow refreshing existing publications as
well whereas, in Add, it will be only for new Publications.

I think the patch v2-0001 will refresh all the publications, I mean
existing and newly added publications. IIUC the patch, it first
fetches all the available publications with the subscriptions and it
sees if that list has the given publication [1], if not, then adds it
to the existing publications list and returns that list [2]. If the
refresh option is specified as true with ALTER SUBSCRIPTION ... ADD
PUBLICATION, then it refreshes all the returned publications [3]. I
believe this is also true with ALTER SUBSCRIPTION ... DROP
PUBLICATION.

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

Yes! It will refresh the new and existing publications.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Wed, 27 Jan 2021 at 16:59, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

If we want add and drop some publications, we can use SET PUBLICATION, it
is more convenient than ADD and DROP PUBLICATION, however if we just want to
add (or drop) publication into (or from) subcription which has much publications,
then the new syntax is more convenient IMO.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Wed, Jan 27, 2021 at 3:26 PM japin <japinli@hotmail.com> wrote:

On Wed, 27 Jan 2021 at 16:59, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jan 26, 2021 at 9:18 AM japin <japinli@hotmail.com> wrote:

When I read the discussion in [1], I found that update subscription's publications
is complicated.

For example, I have 5 publications in subscription.

CREATE SUBSCRIPTION mysub1 CONNECTION 'host=localhost port=5432 dbname=postgres'
PUBLICATION mypub1, mypub2, mypub3, mypub4, mypub5;

If I want to drop "mypub4", we should use the following command:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5;

Also, if I want to add "mypub7" and "mypub8", it will use:

ALTER SUBSCRIPTION mysub1 SET PUBLICATION mypub1, mypub2, mypub3, mypub5, mypub7, mypub8;

Attached implement ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax, for the above
two cases, we can use the following:

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub4;

ALTER SUBSCRIPTION mysub1 DROP PUBLICATION mypub7, mypub8;

I think it's more convenient. Any thoughts?

While the new proposed syntax does seem to provide some ease for users
but it has nothing which we can't do with current syntax. Also, in the
current syntax, there is an additional provision for refreshing the
existing publications as well. So, if the user has to change the
existing subscription such that it has to (a) add new publication(s),
(b) remove some publication(s), (c) refresh existing publication(s)
then all can be done in one command whereas with your new proposed
syntax user has to write three separate commands.

If we want add and drop some publications, we can use SET PUBLICATION, it
is more convenient than ADD and DROP PUBLICATION, however if we just want to
add (or drop) publication into (or from) subcription which has much publications,
then the new syntax is more convenient IMO.

I agree with you that if we just want to add or remove a few
publications in the existing subscription then providing the complete
list is not convenient. The new syntax is way better, although I am
not sure how frequently users need to add/remove publication in the
subscription.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

That sounds a bit unusual to me because when the user has specifically
asked to just ADD Publication, we might refresh some existing
Publication along with it?

--
With Regards,
Amit Kapila.

On Wed, Jan 27, 2021 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

That sounds a bit unusual to me because when the user has specifically
asked to just ADD Publication, we might refresh some existing
Publication along with it?

Hmm. That's correct. I also feel we should not touch the existing
publications, only the ones that are added/dropped should be
refreshed. Because there will be an overhead of a SQL with more
publications(in fetch_table_list) when AlterSubscription_refresh() is
called with all the existing publications. We could just pass in the
newly added/dropped publications to AlterSubscription_refresh().

I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
refresh true refreshes only the newly added publications, because what
we do in AlterSubscription_refresh() is that we fetch the tables
associated with the publications from the publisher, compare them with
the previously fetched tables from that publication and add the new
tables or remove the table that don't exist in that publication
anymore.

For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
thing i.e. refreshes only the dropped publications.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Jan 27, 2021, at 19:41, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

That sounds a bit unusual to me because when the user has specifically
asked to just ADD Publication, we might refresh some existing
Publication along with it?

Hmm. That's correct. I also feel we should not touch the existing
publications, only the ones that are added/dropped should be
refreshed. Because there will be an overhead of a SQL with more
publications(in fetch_table_list) when AlterSubscription_refresh() is
called with all the existing publications. We could just pass in the
newly added/dropped publications to AlterSubscription_refresh().

I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
refresh true refreshes only the newly added publications, because what
we do in AlterSubscription_refresh() is that we fetch the tables
associated with the publications from the publisher, compare them with
the previously fetched tables from that publication and add the new
tables or remove the table that don't exist in that publication
anymore.

For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
thing i.e. refreshes only the dropped publications.

Thoughts?

Agreed. We just only need to refresh the added/dropped publications. Furthermore, for publications that will be dropped, we do not need the “copy_data” option, right?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Jan 27, 2021, at 19:41, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:16 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 3:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

So, I think the new syntax, ALTER SUBSCRIPTION .. ADD/DROP PUBLICATION
will refresh the new and existing publications.

That sounds a bit unusual to me because when the user has specifically
asked to just ADD Publication, we might refresh some existing
Publication along with it?

Hmm. That's correct. I also feel we should not touch the existing
publications, only the ones that are added/dropped should be
refreshed. Because there will be an overhead of a SQL with more
publications(in fetch_table_list) when AlterSubscription_refresh() is
called with all the existing publications. We could just pass in the
newly added/dropped publications to AlterSubscription_refresh().

I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
refresh true refreshes only the newly added publications, because what
we do in AlterSubscription_refresh() is that we fetch the tables
associated with the publications from the publisher, compare them with
the previously fetched tables from that publication and add the new
tables or remove the table that don't exist in that publication
anymore.

For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
thing i.e. refreshes only the dropped publications.

Thoughts?

Argeed. We just only need to refresh the added/dropped publications. Furthermore, for dropped publications we do not need the “copy_data” option, right?

On Wed, Jan 27, 2021 at 7:35 PM Li Japin <japinli@hotmail.com> wrote:

I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
refresh true refreshes only the newly added publications, because what
we do in AlterSubscription_refresh() is that we fetch the tables
associated with the publications from the publisher, compare them with
the previously fetched tables from that publication and add the new
tables or remove the table that don't exist in that publication
anymore.

For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
thing i.e. refreshes only the dropped publications.

Thoughts?

Agreed. We just only need to refresh the added/dropped publications. Furthermore, for publications that will be dropped, we do not need the “copy_data” option, right?

I think you are right. The copy_data option doesn't make sense for
ALTER SUBSCRIPTION ... DROP PUBLICATION, maybe we should throw an
error if the user specifies it. Of course, we need that option for
ALTER SUBSCRIPTION ... ADD PUBLICATION.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, 28 Jan 2021 at 12:22, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Jan 27, 2021 at 7:35 PM Li Japin <japinli@hotmail.com> wrote:

I don't see any problem if ALTER SUBSCRIPTION ... ADD PUBLICATION with
refresh true refreshes only the newly added publications, because what
we do in AlterSubscription_refresh() is that we fetch the tables
associated with the publications from the publisher, compare them with
the previously fetched tables from that publication and add the new
tables or remove the table that don't exist in that publication
anymore.

For ALTER SUBSCRIPTION ... DROP PUBLICATION, also we can do the same
thing i.e. refreshes only the dropped publications.

Thoughts?

Agreed. We just only need to refresh the added/dropped publications. Furthermore, for publications that will be dropped, we do not need the “copy_data” option, right?

I think you are right. The copy_data option doesn't make sense for
ALTER SUBSCRIPTION ... DROP PUBLICATION, maybe we should throw an
error if the user specifies it. Of course, we need that option for
ALTER SUBSCRIPTION ... ADD PUBLICATION.

Thanks for your confirm. Attached v3 patch fix it.

* v3-0001
Only refresh the publications that will be added/dropped, also remove "copy_data"
option from DROP PUBLICATION.

* v3-0002
Add a new testcase for DROP PUBLICATION WITH (copy_data).

* v3-0003
Remove the reference of REFRESH PUBLICATION in DROP PUBLICATION.

* v3-0004
Do not change.

Attaching v3 patches, please consider these for further review.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Jan 28, 2021 at 10:07 AM japin <japinli@hotmail.com> wrote:

Attaching v3 patches, please consider these for further review.

I think we can add a commitfest entry for this feature, so that the
patches will be tested on cfbot. Ignore if done already.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com