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