Horton

Thread

Commits

  1. ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

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

    Japin Li <japinli@hotmail.com> — 2021-01-26T03:47:52Z

    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] - https://www.postgresql.org/message-id/MEYP282MB16690CD5EC5319FC35B2F78AB6BD0%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

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

  2. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-26T03:55:19Z

    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] - https://www.postgresql.org/message-id/MEYP282MB16690CD5EC5319FC35B2F78AB6BD0%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

Sorry, I forgot to attach the patch.

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

  3. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-01-26T04:55:48Z

    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?

I would like to hear opinions from other hackers.

[1] - https://www.postgresql.org/message-id/CALj2ACVGDNZDQk3wfv%3D3zYTg%3DqKUaEa5s1f%2B9_PYxN0QyAUdCw%40mail.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

  4. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-26T05:46:11Z

    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] - https://www.postgresql.org/message-id/CALj2ACVGDNZDQk3wfv%3D3zYTg%3DqKUaEa5s1f%2B9_PYxN0QyAUdCw%40mail.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.

  5. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-26T10:37:33Z

    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] - https://www.postgresql.org/message-id/CALj2ACVGDNZDQk3wfv%3D3zYTg%3DqKUaEa5s1f%2B9_PYxN0QyAUdCw%40mail.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.

  6. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-01-27T08:02:43Z

    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

  7. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-27T08:59:58Z

    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.

  8. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-01-27T09:27:00Z

    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

  9. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-27T09:30:56Z

    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.

  10. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-01-27T09:46:31Z

    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.

[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),

[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

  11. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-27T09:51:30Z

    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.

  12. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-27T09:56:36Z

    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.

  13. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-01-27T10:47:21Z

    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

  14. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Amit Kapila <amit.kapila16@gmail.com> — 2021-01-27T11:12:17Z

    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.

  15. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-01-27T11:41:38Z

    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

  16. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-27T14:05:06Z

    
> 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.

  17. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-01-28T04:22:11Z

    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

  18. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-01-28T04:37:03Z

    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.

  19. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-03T05:15:38Z

    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

  20. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-02-03T08:32:10Z

    On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> 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.
>

Agreed.  I made an entry in the commitfest[1].

[1] - https://commitfest.postgresql.org/32/2965/

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

  21. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-05T09:50:35Z

    On Wed, Feb 3, 2021 at 2:02 PM japin <japinli@hotmail.com> wrote:
> On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > 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.
> >
>
> Agreed.  I made an entry in the commitfest[1].
>
> [1] - https://commitfest.postgresql.org/32/2965/

Thanks. Few comments on 0001 patch:

1) Are we throwing an error if the copy_data option is specified for
DROP? If I'm reading the patch correctly, I think we should let
parse_subscription_options tell whether the copy_data option is
provided irrespective of ADD or DROP, and in case of DROP we should
throw an error outside of parse_subscription_options?

2) What's the significance of the cell == NULL else if clause? IIUC,
when we don't enter +        foreach(cell, publist) or if we enter and
publist has become NULL by then, then the cell can be NULL. If my
understanding is correct, we can move publist == NULL check within the
inner for loop and remote else if (cell == NULL)? Thoughts? If you
have a strong reasong retain this error errmsg("publication name
\"%s\" do not in subscription", then there's a typo
errmsg("publication name \"%s\" does not exists in subscription".

+        else if (cell == NULL)
+            ereport(ERROR,
+                    (errcode(ERRCODE_SYNTAX_ERROR),
+                     errmsg("publication name \"%s\" do not in subscription",
+                            name)));
+    }
+
+    if (publist == NIL)
+        ereport(ERROR,
+                (errcode(ERRCODE_SYNTAX_ERROR),
+                 errmsg("subscription must contain at least one
publication")));

3) In merge_subpublications, instead of doing heap_deform_tuple and
preparing the existing publist ourselves, can't we reuse
textarray_to_stringlist to prepare the publist? Can't we just pass
"tup" and "form" to merge_subpublications and do like below:

    /* Get publications */
    datum = SysCacheGetAttr(SUBSCRIPTIONOID,
                            tup,
                            Anum_pg_subscription_subpublications,
                            &isnull);
    Assert(!isnull);
    publist = textarray_to_stringlist(DatumGetArrayTypeP(datum));

See the code in GetSubscription

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

  22. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-02-05T13:21:37Z

    On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Wed, Feb 3, 2021 at 2:02 PM japin <japinli@hotmail.com> wrote:
>> On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>> > 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.
>> >
>>
>> Agreed.  I made an entry in the commitfest[1].
>>
>> [1] - https://commitfest.postgresql.org/32/2965/
>
> Thanks. Few comments on 0001 patch:
>

Thanks for your reviewing.

> 1) Are we throwing an error if the copy_data option is specified for
> DROP?

Yes, it will throw an error like:

ERROR:  unrecognized subscription parameter: "copy_data"

> If I'm reading the patch correctly, I think we should let
> parse_subscription_options tell whether the copy_data option is
> provided irrespective of ADD or DROP, and in case of DROP we should
> throw an error outside of parse_subscription_options?
>

IIUC, the parse_subscription_options cannot tell us whether the copy_data option
is provided or not.

The comments of parse_subscription_options says:

/*
 * Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
 *
 * Since not all options can be specified in both commands, this function
 * will report an error on options if the target output pointer is NULL to
 * accommodate that.
 */

So I think we can do this for DROP.

> 2) What's the significance of the cell == NULL else if clause? IIUC,
> when we don't enter +        foreach(cell, publist) or if we enter and
> publist has become NULL by then, then the cell can be NULL. If my
> understanding is correct, we can move publist == NULL check within the
> inner for loop and remote else if (cell == NULL)? Thoughts?

We will get cell == NULL when we iterate all items in publist.  I use it
to check whether the dropped publication is in publist or not.

> If you
> have a strong reasong retain this error errmsg("publication name
> \"%s\" do not in subscription", then there's a typo
> errmsg("publication name \"%s\" does not exists in subscription".
>

Fixed.

> +        else if (cell == NULL)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_SYNTAX_ERROR),
> +                     errmsg("publication name \"%s\" do not in subscription",
> +                            name)));
> +    }
> +
> +    if (publist == NIL)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_SYNTAX_ERROR),
> +                 errmsg("subscription must contain at least one
> publication")));
>
> 3) In merge_subpublications, instead of doing heap_deform_tuple and
> preparing the existing publist ourselves, can't we reuse
> textarray_to_stringlist to prepare the publist? Can't we just pass
> "tup" and "form" to merge_subpublications and do like below:
>
>     /* Get publications */
>     datum = SysCacheGetAttr(SUBSCRIPTIONOID,
>                             tup,
>                             Anum_pg_subscription_subpublications,
>                             &isnull);
>     Assert(!isnull);
>     publist = textarray_to_stringlist(DatumGetArrayTypeP(datum));
>
> See the code in GetSubscription
>

Fixed

Attaching v4 patches, please consider these for further review.

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

  23. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-10T13:49:51Z

    On Fri, Feb 5, 2021 at 6:51 PM japin <japinli@hotmail.com> wrote:
> On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> We will get cell == NULL when we iterate all items in publist.  I use it
> to check whether the dropped publication is in publist or not.
>
> > If you
> > have a strong reasong retain this error errmsg("publication name
> > \"%s\" do not in subscription", then there's a typo
> > errmsg("publication name \"%s\" does not exists in subscription".
>
> Fixed.

I think we still have a typo in 0002, it's
+                     errmsg("publication name \"%s\" does not exist
in subscription",
instead of
+                     errmsg("publication name \"%s\" does not exists
in subscription",

IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
errors out on the first publication that already exists/that doesn't
exist right? What if there are multiple publications given in the
ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
loop over the subscription's publication list and show all the already
existing/not existing publications in the error message, instead of
just erroring out for the first existing/not existing publication?

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

  24. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-02-13T06:11:23Z

    Thanks for your review again.

On Wed, 10 Feb 2021 at 21:49, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Fri, Feb 5, 2021 at 6:51 PM japin <japinli@hotmail.com> wrote:
>> On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>> We will get cell == NULL when we iterate all items in publist.  I use it
>> to check whether the dropped publication is in publist or not.
>>
>> > If you
>> > have a strong reasong retain this error errmsg("publication name
>> > \"%s\" do not in subscription", then there's a typo
>> > errmsg("publication name \"%s\" does not exists in subscription".
>>
>> Fixed.
>
> I think we still have a typo in 0002, it's
> +                     errmsg("publication name \"%s\" does not exist
> in subscription",
> instead of
> +                     errmsg("publication name \"%s\" does not exists
> in subscription",
>

Fixed.

> IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
> errors out on the first publication that already exists/that doesn't
> exist right? What if there are multiple publications given in the
> ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
> loop over the subscription's publication list and show all the already
> existing/not existing publications in the error message, instead of
> just erroring out for the first existing/not existing publication?
>

Yes, you are right. Agree with you, I modified it. Please consider v5
for further review.

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

  25. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-15T02:43:49Z

    On Sat, Feb 13, 2021 at 11:41 AM japin <japinli@hotmail.com> wrote:
> > IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
> > errors out on the first publication that already exists/that doesn't
> > exist right? What if there are multiple publications given in the
> > ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
> > loop over the subscription's publication list and show all the already
> > existing/not existing publications in the error message, instead of
> > just erroring out for the first existing/not existing publication?
> >
>
> Yes, you are right. Agree with you, I modified it. Please consider v5
> for further review.

Thanks for the updated patches. I have a comment about reporting the
existing/not existing publications code. How about something like the
attached delta patch on v5-0002? Sorry for attaching

I also think that we could merge 0002 into 0001 and have only 4
patches in the patch set.

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

  26. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-02-16T01:58:13Z

    On Mon, Feb 15, 2021 at 8:13 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, Feb 13, 2021 at 11:41 AM japin <japinli@hotmail.com> wrote:
> > > IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
> > > errors out on the first publication that already exists/that doesn't
> > > exist right? What if there are multiple publications given in the
> > > ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
> > > loop over the subscription's publication list and show all the already
> > > existing/not existing publications in the error message, instead of
> > > just erroring out for the first existing/not existing publication?
> > >
> >
> > Yes, you are right. Agree with you, I modified it. Please consider v5
> > for further review.
>
> Thanks for the updated patches. I have a comment about reporting the
> existing/not existing publications code. How about something like the
> attached delta patch on v5-0002?

Attaching the v6 patch set so that cfbot can proceed to test the
patches. The above delta patch was merged into 0002. Please have a
look.

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

  27. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-02-18T02:31:14Z

    On Tue, 16 Feb 2021 at 09:58, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Mon, Feb 15, 2021 at 8:13 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Sat, Feb 13, 2021 at 11:41 AM japin <japinli@hotmail.com> wrote:
>> > > IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
>> > > errors out on the first publication that already exists/that doesn't
>> > > exist right? What if there are multiple publications given in the
>> > > ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
>> > > loop over the subscription's publication list and show all the already
>> > > existing/not existing publications in the error message, instead of
>> > > just erroring out for the first existing/not existing publication?
>> > >
>> >
>> > Yes, you are right. Agree with you, I modified it. Please consider v5
>> > for further review.
>>
>> Thanks for the updated patches. I have a comment about reporting the
>> existing/not existing publications code. How about something like the
>> attached delta patch on v5-0002?
>
> Attaching the v6 patch set so that cfbot can proceed to test the
> patches. The above delta patch was merged into 0002. Please have a
> look.
>

Thanks for the updated patches. I'm on vacation.

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

  28. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-07T11:43:42Z

    On Thu, Feb 18, 2021 at 8:01 AM japin <japinli@hotmail.com> wrote:
> On Tue, 16 Feb 2021 at 09:58, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> > On Mon, Feb 15, 2021 at 8:13 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> On Sat, Feb 13, 2021 at 11:41 AM japin <japinli@hotmail.com> wrote:
> >> > > IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
> >> > > errors out on the first publication that already exists/that doesn't
> >> > > exist right? What if there are multiple publications given in the
> >> > > ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
> >> > > loop over the subscription's publication list and show all the already
> >> > > existing/not existing publications in the error message, instead of
> >> > > just erroring out for the first existing/not existing publication?
> >> > >
> >> >
> >> > Yes, you are right. Agree with you, I modified it. Please consider v5
> >> > for further review.
> >>
> >> Thanks for the updated patches. I have a comment about reporting the
> >> existing/not existing publications code. How about something like the
> >> attached delta patch on v5-0002?
> >
> > Attaching the v6 patch set so that cfbot can proceed to test the
> > patches. The above delta patch was merged into 0002. Please have a
> > look.
> >
>
> Thanks for the updated patches. I'm on vacation.

I'm reading through the v6 patches again, here are some comments.

1) IMO, we can merge 0001 into 0002
2) A typo, it's "current" not "ccurrent" - + * Merge ccurrent
subscription's publications and user specified publications
3) In merge_subpublications, do we need to error out or do something
instead of Assert(!isnull); as in the release build we don't reach
assert. So, if at all catalogue search returns a null tuple, we don't
surprise users.
4) Can we have a better name for the function merge_subpublications
say merge_publications (because it's a local function to
subscriptioncmds.c we don't need "sub" in function name) or any other
better name?
5) Instead of doing catalogue look up for the subscriber publications
in merge_subpublications, why can't we pass in the list from sub =
GetSubscription(subid, false); (being called in AlterSubscription)
---> sub->publications. Do you see any problems in doing so? If done
that, we can discard the 0001 patch and comments (1) and (3) becomes
irrelevant.

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

  29. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-03-07T13:50:41Z

    On Sun, 07 Mar 2021 at 19:43, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> I'm reading through the v6 patches again, here are some comments.
>

Thanks for your review again.

> 1) IMO, we can merge 0001 into 0002
> 2) A typo, it's "current" not "ccurrent" - + * Merge ccurrent
> subscription's publications and user specified publications

Fixed.

> 3) In merge_subpublications, do we need to error out or do something
> instead of Assert(!isnull); as in the release build we don't reach
> assert. So, if at all catalogue search returns a null tuple, we don't
> surprise users.
> 4) Can we have a better name for the function merge_subpublications
> say merge_publications (because it's a local function to
> subscriptioncmds.c we don't need "sub" in function name) or any other
> better name?

Rename merge_subpublications to merge_publications as you suggested.

> 5) Instead of doing catalogue look up for the subscriber publications
> in merge_subpublications, why can't we pass in the list from sub =
> GetSubscription(subid, false); (being called in AlterSubscription)
> ---> sub->publications. Do you see any problems in doing so? If done
> that, we can discard the 0001 patch and comments (1) and (3) becomes
> irrelevant.

Thank you point out this.  Fixed it in v7 patch set.

Please consider the v7 patch for futher review.

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

  30. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-22T03:14:29Z

    On Sun, Mar 7, 2021 at 7:21 PM Japin Li <japinli@hotmail.com> wrote:
> Thank you point out this.  Fixed it in v7 patch set.
>
> Please consider the v7 patch for futher review.

Thanks for the patches. I just found the following behaviour with the
new ADD/DROP syntax: when the specified publication list has
duplicates, the patch is throwing "publication is already present"
error. It's adding the first instance of the duplicate into the list
and the second instance is being checked in the added list and
throwing the "already present error". The error message means that the
publication is already present in the subscription but it's not true.
See my testing at [1].

I think we have two cases:
case 1: the publication/s specified in the new ADD/DROP syntax may/may
not have already been associated with the subscription, so the error
"publication is already present"/"publication doesn't exist" error
makes sense.
case 2: there can be duplicate publications specified in the new
ADD/DROP syntax, in this case the error "publication name "mypub2"
used more than once" makes more sense much like [2].

[1]
postgres=# select subpublications from pg_subscription;
 subpublications
-----------------
 {mypub,mypub1}

postgres=# alter subscription mysub add publication mypub2, mypub2;
ERROR:  publication "mypub2" is already present in the subscription

postgres=# select subpublications from pg_subscription;
    subpublications
-----------------------
 {mypub,mypub1,mypub2}

postgres=# alter subscription mysub drop publication mypub2, mypub2;
ERROR:  publication "mypub2" doesn't exist in the subscription

[2]
postgres=# alter subscription mysub set publication mypub2, mypub2;
ERROR:  publication name "mypub2" used more than once

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

  31. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-03-23T15:08:43Z

    On Mon, 22 Mar 2021 at 11:14, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Sun, Mar 7, 2021 at 7:21 PM Japin Li <japinli@hotmail.com> wrote:
>> Thank you point out this.  Fixed it in v7 patch set.
>>
>> Please consider the v7 patch for futher review.
>
> Thanks for the patches. I just found the following behaviour with the
> new ADD/DROP syntax: when the specified publication list has
> duplicates, the patch is throwing "publication is already present"
> error. It's adding the first instance of the duplicate into the list
> and the second instance is being checked in the added list and
> throwing the "already present error". The error message means that the
> publication is already present in the subscription but it's not true.
> See my testing at [1].
>
> I think we have two cases:
> case 1: the publication/s specified in the new ADD/DROP syntax may/may
> not have already been associated with the subscription, so the error
> "publication is already present"/"publication doesn't exist" error
> makes sense.
> case 2: there can be duplicate publications specified in the new
> ADD/DROP syntax, in this case the error "publication name "mypub2"
> used more than once" makes more sense much like [2].
>
> [1]
> postgres=# select subpublications from pg_subscription;
>  subpublications
> -----------------
>  {mypub,mypub1}
>
> postgres=# alter subscription mysub add publication mypub2, mypub2;
> ERROR:  publication "mypub2" is already present in the subscription
>
> postgres=# select subpublications from pg_subscription;
>     subpublications
> -----------------------
>  {mypub,mypub1,mypub2}
>
> postgres=# alter subscription mysub drop publication mypub2, mypub2;
> ERROR:  publication "mypub2" doesn't exist in the subscription
>
> [2]
> postgres=# alter subscription mysub set publication mypub2, mypub2;
> ERROR:  publication name "mypub2" used more than once
>

Thanks for your review.

I check the duplicates for newpublist in merge_publications(). The code is
copied from publicationListToArray().

I do not check for all duplicates because it will make the code more complex.
For example:

ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;

If we record the duplicate publication names in list A, when we find a
duplication in newpublist, we should check whether the publication is
in list A or not, to make the error message make sense (do not have
duplicate publication names in error message).

Thought?

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

  32. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-01T15:53:12Z

    On Tue, Mar 23, 2021 at 8:39 PM Japin Li <japinli@hotmail.com> wrote:
> I check the duplicates for newpublist in merge_publications(). The code is
> copied from publicationListToArray().

IMO, we can have the same code inside a function, probably named
"check_duplicates_in_publist" or some other better name:
static void
check_duplicates_in_publist(List *publist, Datum *datums)
{
    int    j = 0;
    ListCell   *cell;

    foreach(cell, publist)
    {
        char       *name = strVal(lfirst(cell));
        ListCell   *pcell;

        /* Check for duplicates. */
        foreach(pcell, publist)
        {
            char       *pname = strVal(lfirst(pcell));

            if (pcell == cell)
                break;

            if (strcmp(name, pname) == 0)
                ereport(ERROR,
                        (errcode(ERRCODE_SYNTAX_ERROR),
                         errmsg("publication name \"%s\" used more than once",
                                pname)));
        }

        if (datums)
            datums[j++] = CStringGetTextDatum(name);
    }
}

From publicationListToArray, call check_duplicates_in_publist(publist, datums);
>From merge_publications, call check_duplicates_in_publist(newpublist, NULL);

> I do not check for all duplicates because it will make the code more complex.
> For example:
>
> ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;

That's fine because we anyways, error out.

0002:
The tests added in subscription.sql look fine to me and they cover
most of the syntax related code. But it will be good if we can add
tests to see if the data of the newly added/dropped publications
would/would not reflect on the subscriber, maybe you can consider
adding these tests into 001_rep_changes.pl, similar to ALTER
SUBSCRIPTION ... SET PUBLICATION test there.

0003:
I think it's not "set_publication_option", they are
"add_publication_option" and "drop_publication_option" for ADD and
DROP respectively. Please change it wherever "set_publication_option"
is used instead.
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
ADD PUBLICATION <replaceable
class="parameter">publication_name</replaceable> [, ...] [ WITH (
<replaceable class="parameter">set_publication_option</replaceable> [=
<replaceable class="parameter">value</replaceable>] [, ... ] ) ]
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
DROP PUBLICATION <replaceable
class="parameter">publication_name</replaceable> [, ...] [ WITH (
<replaceable class="parameter">set_publication_option</replaceable> [=
<replaceable class="parameter">value</replaceable>] [, ... ] ) ]

0004:
LGTM.

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

  33. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-02T19:59:13Z

    On 23.03.21 16:08, Japin Li wrote:
> I check the duplicates for newpublist in merge_publications(). The code is
> copied from publicationListToArray().
> 
> I do not check for all duplicates because it will make the code more complex.
> For example:
> 
> ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;
> 
> If we record the duplicate publication names in list A, when we find a
> duplication in newpublist, we should check whether the publication is
> in list A or not, to make the error message make sense (do not have
> duplicate publication names in error message).

The code you have in merge_publications() to report all existing 
publications is pretty messy and is not properly internationalized.  I 
think what you are trying to do there is excessive.  Compare this 
similar case:

create table t1 (a int, b int);
alter table t1 add column a int, add column b int;
ERROR:  42701: column "a" of relation "t1" already exists

I think you can make both this and the duplicate checking much simpler 
if you just report the first conflict.

I think this patch is about ready to commit, but please provide a final 
version in good time.

(Also, please combine your patches into a single patch.)

  34. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-03T05:20:46Z

    On Sat, Apr 3, 2021 at 1:29 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> The code you have in merge_publications() to report all existing
> publications is pretty messy and is not properly internationalized.  I
> think what you are trying to do there is excessive.  Compare this
> similar case:
>
> create table t1 (a int, b int);
> alter table t1 add column a int, add column b int;
> ERROR:  42701: column "a" of relation "t1" already exists
>
> I think you can make both this and the duplicate checking much simpler
> if you just report the first conflict.

Yes, we are erroring out on the first conflict for both duplicates and
in merge_publications.

> I think this patch is about ready to commit, but please provide a final
> version in good time.

I took the liberty to address all the review comments and provide a v9
patch on top of Japin's v8 patch-set.

> (Also, please combine your patches into a single patch.)

Done.

Attaching v9 patch, please review it.

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

  35. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-04-06T05:24:23Z

    On Sat, 03 Apr 2021 at 13:20, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Sat, Apr 3, 2021 at 1:29 AM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> The code you have in merge_publications() to report all existing
>> publications is pretty messy and is not properly internationalized.  I
>> think what you are trying to do there is excessive.  Compare this
>> similar case:
>>
>> create table t1 (a int, b int);
>> alter table t1 add column a int, add column b int;
>> ERROR:  42701: column "a" of relation "t1" already exists
>>
>> I think you can make both this and the duplicate checking much simpler
>> if you just report the first conflict.
>
> Yes, we are erroring out on the first conflict for both duplicates and
> in merge_publications.
>
>> I think this patch is about ready to commit, but please provide a final
>> version in good time.
>
> I took the liberty to address all the review comments and provide a v9
> patch on top of Japin's v8 patch-set.
>
>> (Also, please combine your patches into a single patch.)
>
> Done.
>
> Attaching v9 patch, please review it.
>

Sorry for the late reply! Thanks for your updating the new patch, and it looks
good to me.

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

  36. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-06T09:56:32Z

    On 06.04.21 07:24, Japin Li wrote:
>>> I think this patch is about ready to commit, but please provide a final
>>> version in good time.
>> I took the liberty to address all the review comments and provide a v9
>> patch on top of Japin's v8 patch-set.
>>
>>> (Also, please combine your patches into a single patch.)
>> Done.
>>
>> Attaching v9 patch, please review it.
>>
> Sorry for the late reply! Thanks for your updating the new patch, and it looks
> good to me.

Committed.

Note that you can use syntax like "ADD|DROP|SET" in the tab completion 
code.  I have simplified some of your code like that.

I also deduplicated the documentation additions a bit.

  37. Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

    Japin Li <japinli@hotmail.com> — 2021-04-07T02:49:02Z

    On Tue, 06 Apr 2021 at 17:56, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> On 06.04.21 07:24, Japin Li wrote:
>>>> I think this patch is about ready to commit, but please provide a final
>>>> version in good time.
>>> I took the liberty to address all the review comments and provide a v9
>>> patch on top of Japin's v8 patch-set.
>>>
>>>> (Also, please combine your patches into a single patch.)
>>> Done.
>>>
>>> Attaching v9 patch, please review it.
>>>
>> Sorry for the late reply! Thanks for your updating the new patch, and it looks
>> good to me.
>
> Committed.
>
> Note that you can use syntax like "ADD|DROP|SET" in the tab completion
> code.  I have simplified some of your code like that.
>
> I also deduplicated the documentation additions a bit.

Thanks!

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