Hi, Bharath
Thanks for your reviewing. On Tue, 26 Jan 2021 at 12:55, Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > On Tue, Jan 26, 2021 at 9:25 AM japin <japi...@hotmail.com> wrote: >> > I think it's more convenient. Any thoughts? >> >> Sorry, I forgot to attach the patch. > > As I mentioned earlier in [1], +1 from my end to have the new syntax > for adding/dropping publications from subscriptions i.e. ALTER > SUBSCRIPTION ... ADD/DROP PUBLICATION. But I'm really not sure why > that syntax was not added earlier. Are we missing something here? > Yeah, we should figure out why we do not support this syntax earlier. It seems ALTER SUBSCRIPTION is introduced in 665d1fad99e, however the commit do not contains any discussion URL. > I would like to hear opinions from other hackers. > > [1] - > https://www.postgresql.org/message-id/CALj2ACVGDNZDQk3wfv%3D3zYTg%3DqKUaEa5s1f%2B9_PYxN0QyAUdCw%40mail.gmail.com > > Some quick comments on the patch, although I have not taken a deeper look at > it: > > 1. IMO, it will be good if the patch is divided into say coding, test > cases and documentation Agreed. I will refactor it in next patch. > 2. Looks like AlterSubscription() is already having ~200 LOC, why > can't we have separate functions for each ALTER_SUBSCRIPTION_XXXX case > or at least for the new code that's getting added for this patch? I'm not sure it is necessary to provide a separate functions for each ALTER_SUBSCRIPTION_XXX, so I just followed current style. > 3. The new code added for ALTER_SUBSCRIPTION_ADD_PUBLICATION and > ALTER_SUBSCRIPTION_DROP_PUBLICATION looks almost same maybe with > little difference, so why can't we have single function > (alter_subscription_add_or_drop_publication or > hanlde_add_or_drop_publication or some better name?) and pass in a > flag to differentiate the code that differs for both commands. Agreed. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
