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

On Tue, Mar 23, 2021 at 8:39 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
> I check the duplicates for newpublist in merge_publications(). The code is
> copied from publicationListToArray().

IMO, we can have the same code inside a function, probably named
"check_duplicates_in_publist" or some other better name:
static void
check_duplicates_in_publist(List *publist, Datum *datums)
{
int j = 0;
ListCell *cell;

foreach(cell, publist)
{
char *name = strVal(lfirst(cell));
ListCell *pcell;

/* Check for duplicates. */
foreach(pcell, publist)
{
char *pname = strVal(lfirst(pcell));

if (pcell == cell)
break;

if (strcmp(name, pname) == 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("publication name \"%s\" used more than once",
pname)));
}

if (datums)
datums[j++] = CStringGetTextDatum(name);
}
}

From publicationListToArray, call check_duplicates_in_publist(publist, datums);
From merge_publications, call check_duplicates_in_publist(newpublist, NULL);

> I do not check for all duplicates because it will make the code more complex.
> For example:
>
> ALTER SUBSCRIPTION mysub ADD PUBLICATION mypub2, mypub2, mypub2;

That's fine because we anyways, error out.

0002:
The tests added in subscription.sql look fine to me and they cover
most of the syntax related code. But it will be good if we can add
tests to see if the data of the newly added/dropped publications
would/would not reflect on the subscriber, maybe you can consider
adding these tests into 001_rep_changes.pl, similar to ALTER
SUBSCRIPTION ... SET PUBLICATION test there.

0003:
I think it's not "set_publication_option", they are
"add_publication_option" and "drop_publication_option" for ADD and
DROP respectively. Please change it wherever "set_publication_option"
is used instead.
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
ADD PUBLICATION <replaceable
class="parameter">publication_name</replaceable> [, ...] [ WITH (
<replaceable class="parameter">set_publication_option</replaceable> [=
<replaceable class="parameter">value</replaceable>] [, ... ] ) ]
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
DROP PUBLICATION <replaceable
class="parameter">publication_name</replaceable> [, ...] [ WITH (
<replaceable class="parameter">set_publication_option</replaceable> [=
<replaceable class="parameter">value</replaceable>] [, ... ] ) ]

0004:
LGTM.

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