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

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:

1) Are we throwing an error if the copy_data option is specified for
DROP? 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?

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? 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".

+ 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

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com