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

On Mon, 22 Mar 2021 at 11:14, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Sun, Mar 7, 2021 at 7:21 PM Japin Li <japinli@hotmail.com> wrote:
>> Thank you point out this.  Fixed it in v7 patch set.
>>
>> Please consider the v7 patch for futher review.
>
> Thanks for the patches. I just found the following behaviour with the
> new ADD/DROP syntax: when the specified publication list has
> duplicates, the patch is throwing "publication is already present"
> error. It's adding the first instance of the duplicate into the list
> and the second instance is being checked in the added list and
> throwing the "already present error". The error message means that the
> publication is already present in the subscription but it's not true.
> See my testing at [1].
>
> I think we have two cases:
> case 1: the publication/s specified in the new ADD/DROP syntax may/may
> not have already been associated with the subscription, so the error
> "publication is already present"/"publication doesn't exist" error
> makes sense.
> case 2: there can be duplicate publications specified in the new
> ADD/DROP syntax, in this case the error "publication name "mypub2"
> used more than once" makes more sense much like [2].
>
> [1]
> postgres=# select subpublications from pg_subscription;
>  subpublications
> -----------------
>  {mypub,mypub1}
>
> postgres=# alter subscription mysub add publication mypub2, mypub2;
> ERROR:  publication "mypub2" is already present in the subscription
>
> postgres=# select subpublications from pg_subscription;
>     subpublications
> -----------------------
>  {mypub,mypub1,mypub2}
>
> postgres=# alter subscription mysub drop publication mypub2, mypub2;
> ERROR:  publication "mypub2" doesn't exist in the subscription
>
> [2]
> postgres=# alter subscription mysub set publication mypub2, mypub2;
> ERROR:  publication name "mypub2" used more than once
>

Thanks for your review.

I check the duplicates for newpublist in merge_publications(). The code is
copied from publicationListToArray().

I do not check for all duplicates because it will make the code more complex.
For example:

ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;

If we record the duplicate publication names in list A, when we find a
duplication in newpublist, we should check whether the publication is
in list A or not, to make the error message make sense (do not have
duplicate publication names in error message).

Thought?

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