On Thu, Feb 18, 2021 at 8:01 AM japin <japi...@hotmail.com> wrote: > On Tue, 16 Feb 2021 at 09:58, Bharath Rupireddy > <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Mon, Feb 15, 2021 at 8:13 AM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > >> > >> On Sat, Feb 13, 2021 at 11:41 AM japin <japi...@hotmail.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
