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.