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

On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> On Wed, Feb 3, 2021 at 2:02 PM japin <japinli(at)hotmail(dot)com> wrote:
>> On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>> > On Thu, Jan 28, 2021 at 10:07 AM japin <japinli(at)hotmail(dot)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.