Thread: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax
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.
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.
Attachment
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
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.
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.
Attachment
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], 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
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 needthe “copy_data” option, right? > With Regards, > Bharath Rupireddy. > EnterpriseDB: http://www.enterprisedb.com
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.
Attachment
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
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.
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
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.
Attachment
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
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.
Attachment
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
Attachment
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
Attachment
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.
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
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.
Attachment
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
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.)
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
Attachment
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.
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.
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.