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

On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Wed, Feb 3, 2021 at 2:02 PM japin <japinli@hotmail.com> wrote:
>> On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>> > On Thu, Jan 28, 2021 at 10:07 AM japin <japinli@hotmail.com> wrote:
>> >> Attaching v3 patches, please consider these for further review.
>> >
>> > I think we can add a commitfest entry for this feature, so that the
>> > patches will be tested on cfbot. Ignore if done already.
>> >
>>
>> Agreed.  I made an entry in the commitfest[1].
>>
>> [1] - https://commitfest.postgresql.org/32/2965/
>
> Thanks. Few comments on 0001 patch:
>

Thanks for your reviewing.

> 1) Are we throwing an error if the copy_data option is specified for
> DROP?

Yes, it will throw an error like:

ERROR:  unrecognized subscription parameter: "copy_data"

> If I'm reading the patch correctly, I think we should let
> parse_subscription_options tell whether the copy_data option is
> provided irrespective of ADD or DROP, and in case of DROP we should
> throw an error outside of parse_subscription_options?
>

IIUC, the parse_subscription_options cannot tell us whether the copy_data option
is provided or not.

The comments of parse_subscription_options says:

/*
 * Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
 *
 * Since not all options can be specified in both commands, this function
 * will report an error on options if the target output pointer is NULL to
 * accommodate that.
 */

So I think we can do this for DROP.

> 2) What's the significance of the cell == NULL else if clause? IIUC,
> when we don't enter +        foreach(cell, publist) or if we enter and
> publist has become NULL by then, then the cell can be NULL. If my
> understanding is correct, we can move publist == NULL check within the
> inner for loop and remote else if (cell == NULL)? Thoughts?

We will get cell == NULL when we iterate all items in publist.  I use it
to check whether the dropped publication is in publist or not.

> If you
> have a strong reasong retain this error errmsg("publication name
> \"%s\" do not in subscription", then there's a typo
> errmsg("publication name \"%s\" does not exists in subscription".
>

Fixed.

> +        else if (cell == NULL)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_SYNTAX_ERROR),
> +                     errmsg("publication name \"%s\" do not in subscription",
> +                            name)));
> +    }
> +
> +    if (publist == NIL)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_SYNTAX_ERROR),
> +                 errmsg("subscription must contain at least one
> publication")));
>
> 3) In merge_subpublications, instead of doing heap_deform_tuple and
> preparing the existing publist ourselves, can't we reuse
> textarray_to_stringlist to prepare the publist? Can't we just pass
> "tup" and "form" to merge_subpublications and do like below:
>
>     /* Get publications */
>     datum = SysCacheGetAttr(SUBSCRIPTIONOID,
>                             tup,
>                             Anum_pg_subscription_subpublications,
>                             &isnull);
>     Assert(!isnull);
>     publist = textarray_to_stringlist(DatumGetArrayTypeP(datum));
>
> See the code in GetSubscription
>

Fixed

Attaching v4 patches, please consider these for further review.

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