Re: alter subscription drop publication fixes - Mailing list pgsql-hackers

From Japin Li
Subject Re: alter subscription drop publication fixes
Date
Msg-id MEYP282MB166999A2AD5CBFCEC6816BFAB6519@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Whole thread Raw
In response to Re: alter subscription drop publication fixes  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: alter subscription drop publication fixes
List pgsql-hackers
On Thu, 13 May 2021 at 00:45, 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.
>

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



pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: PG 14 release notes, first draft
Next
From: Julien Rouhaud
Date:
Subject: Re: compute_query_id and pg_stat_statements