Re: alter subscription drop publication fixes

On Thu, 13 May 2021 at 22:13, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> On Wed, May 12, 2021 at 10:15 PM 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.
>>
>> > 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'.

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