Re: alter subscription drop publication fixes

On Thu, 13 May 2021 at 00:45, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21(at)gmail(dot)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.
>

+1. Make sense to remove the unsupported options for tab-complete.

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

Agree with Bharath. We can use a detail message. How about?

if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("subscription must contain at least one publication"),
errdetail("Dropping all the publications from a subscription is not supported")));

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

+1.

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