Re: Support ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION ... syntax

On Sun, 07 Mar 2021 at 19:43, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> I'm reading through the v6 patches again, here are some comments.
>

Thanks for your review again.

> 1) IMO, we can merge 0001 into 0002
> 2) A typo, it's "current" not "ccurrent" - + * Merge ccurrent
> subscription's publications and user specified publications

Fixed.

> 3) In merge_subpublications, do we need to error out or do something
> instead of Assert(!isnull); as in the release build we don't reach
> assert. So, if at all catalogue search returns a null tuple, we don't
> surprise users.
> 4) Can we have a better name for the function merge_subpublications
> say merge_publications (because it's a local function to
> subscriptioncmds.c we don't need "sub" in function name) or any other
> better name?

Rename merge_subpublications to merge_publications as you suggested.

> 5) Instead of doing catalogue look up for the subscriber publications
> in merge_subpublications, why can't we pass in the list from sub =
> GetSubscription(subid, false); (being called in AlterSubscription)
> ---> sub->publications. Do you see any problems in doing so? If done
> that, we can discard the 0001 patch and comments (1) and (3) becomes
> irrelevant.

Thank you point out this.  Fixed it in v7 patch set.

Please consider the v7 patch for futher review.

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