On Fri, May 14, 2021 at 7:41 AM Japin Li <japi...@hotmail.com> wrote: > > > On Thu, 13 May 2021 at 22:13, vignesh C <vignes...@gmail.com> wrote: > > On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > >> > >> On Wed, May 12, 2021 at 9:55 PM vignesh C <vignes...@gmail.com> wrote: > >> > While I was reviewing one of the logical decoding features, I found a > >> > few issues in alter subscription drop publication. > >> > >> Thanks! > >> > >> > Alter subscription drop publication does not support copy_data option, > >> > that needs to be removed from tab completion. > >> > >> +1. You may want to also change set_publication_option(to something > >> like drop_pulication_option with only refresh option) for the drop in > >> the docs? Because "Additionally, refresh options as described under > >> REFRESH PUBLICATION may be specified." doesn't make sense. > >> > >> > Dropping all the publications present in the subscription using alter > >> > subscription drop publication would throw "subscription must contain > >> > at least one publication". This message was slightly confusing to me. > >> > As even though some publication was present on the subscription I was > >> > not able to drop. Instead I feel we could throw an error message > >> > something like "dropping specified publication will result in > >> > subscription without any publication, this is not supported". > >> > >> -1 for that long message. The intention of that error was to throw an > >> error if all the publications of a subscription are dropped. If that's > >> so confusing, then you could just let the error message be > >> "subscription must contain at least one publication", add an error > >> detail "Subscription without any publication is not allowed to > >> exist/is not supported." or "Removing/Dropping all the publications > >> from a subscription is not allowed/supported." or some other better > >> wording. > >> > > > > Modified the error message to "errmsg("cannot drop all the > > publications of the subscriber \"%s\"", subname)". > > I have separated the Drop publication documentation contents. There > > are some duplicate contents but the readability is slightly better. > > Thoughts? > > > >> > merge_publications can be called after validation of the options > >> > specified, I think we should check if the options specified are > >> > correct or not before checking the actual publications. > >> > >> +1. That was a miss in the original feature. > > > > Attached patch has the changes for the same. > > > > Thanks for updating the patch. I have a little comments for the new patch. > > - <literal>ADD</literal> adds additional publications, > - <literal>DROP</literal> removes publications from the list of > + <literal>ADD</literal> adds additional publications from the list of > > I think, we should change the word 'from' to 'to'.
I have changed it to: <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>DROP</literal> removes publications to/from the list of The changes for the same are shared in v3 patch at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm3svMg%2BhMA9GsJsUQ75HXtpjpAh2gk%3D8yZfgAnA9BMsnA%40mail.gmail.com Regards, Vignesh