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

On Thu, Feb 18, 2021 at 8:01 AM japin <japinli(at)hotmail(dot)com> wrote:
> On Tue, 16 Feb 2021 at 09:58, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > On Mon, Feb 15, 2021 at 8:13 AM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >>
> >> On Sat, Feb 13, 2021 at 11:41 AM japin <japinli(at)hotmail(dot)com> wrote:
> >> > > IIUC, with the current patch, the new ALTER SUBSCRIPTION ... ADD/DROP
> >> > > errors out on the first publication that already exists/that doesn't
> >> > > exist right? What if there are multiple publications given in the
> >> > > ADD/DROP list, and few of them exist/don't exist. Isn't it good if we
> >> > > loop over the subscription's publication list and show all the already
> >> > > existing/not existing publications in the error message, instead of
> >> > > just erroring out for the first existing/not existing publication?
> >> > >
> >> >
> >> > Yes, you are right. Agree with you, I modified it. Please consider v5
> >> > for further review.
> >>
> >> Thanks for the updated patches. I have a comment about reporting the
> >> existing/not existing publications code. How about something like the
> >> attached delta patch on v5-0002?
> >
> > Attaching the v6 patch set so that cfbot can proceed to test the
> > patches. The above delta patch was merged into 0002. Please have a
> > look.
> >
>
> Thanks for the updated patches. I'm on vacation.

I'm reading through the v6 patches again, here are some comments.

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

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com