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.
Regards,
Vignesh