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

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:

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