Re: alter subscription drop publication fixes

On Fri, May 14, 2021 at 7:41 AM Japin Li <japinli@hotmail.com> wrote:
>
>
> On Thu, 13 May 2021 at 22:13, vignesh C <vignesh21@gmail.com> wrote:
> > On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >>
> >> On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@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