On Tue, 26 Jan 2021 at 13:46, japin <japi...@hotmail.com> wrote:
> Hi, Bharath
>
> Thanks for your reviewing.
>
> On Tue, 26 Jan 2021 at 12:55, Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
>> On Tue, Jan 26, 2021 at 9:25 AM japin <japi...@hotmail.com> wrote:
>>> > I think it's more convenient. Any thoughts?
>>>
>>> Sorry, I forgot to attach the patch.
>>
>> As I mentioned earlier in [1], +1 from my end to have the new syntax
>> for adding/dropping publications from subscriptions i.e. ALTER
>> SUBSCRIPTION ... ADD/DROP PUBLICATION. But I'm really not sure why
>> that syntax was not added earlier. Are we missing something here?
>>
>
> Yeah, we should figure out why we do not support this syntax earlier. It
> seems
> ALTER SUBSCRIPTION is introduced in 665d1fad99e, however the commit do not
> contains any discussion URL.
>
>> I would like to hear opinions from other hackers.
>>
>> [1] -
>> https://www.postgresql.org/message-id/CALj2ACVGDNZDQk3wfv%3D3zYTg%3DqKUaEa5s1f%2B9_PYxN0QyAUdCw%40mail.gmail.com
>>
>> Some quick comments on the patch, although I have not taken a deeper look at
>> it:
>>
>> 1. IMO, it will be good if the patch is divided into say coding, test
>> cases and documentation
>
> Agreed. I will refactor it in next patch.
>
Split v1 path into coding, test cases, documentation and tab-complete.
>> 2. Looks like AlterSubscription() is already having ~200 LOC, why
>> can't we have separate functions for each ALTER_SUBSCRIPTION_XXXX case
>> or at least for the new code that's getting added for this patch?
>
> I'm not sure it is necessary to provide a separate functions for each
> ALTER_SUBSCRIPTION_XXX, so I just followed current style.
>
>> 3. The new code added for ALTER_SUBSCRIPTION_ADD_PUBLICATION and
>> ALTER_SUBSCRIPTION_DROP_PUBLICATION looks almost same maybe with
>> little difference, so why can't we have single function
>> (alter_subscription_add_or_drop_publication or
>> hanlde_add_or_drop_publication or some better name?) and pass in a
>> flag to differentiate the code that differs for both commands.
>
> Agreed.
At present, I create a new function merge_subpublications() to merge the origin
publications and add/drop publications. Thoughts?
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From 5af6cc67938b31b39fa1998a10a9c7f1bdd8fb0e Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 15:43:52 +0800
Subject: [PATCH v2 1/4] Add ALTER SUBSCRIPTION...ADD/DROP PUBLICATION...
syntax
---
src/backend/commands/subscriptioncmds.c | 119 ++++++++++++++++++++++++
src/backend/parser/gram.y | 20 ++++
src/include/nodes/parsenodes.h | 2 +
3 files changed, 141 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 082f7855b8..07167c9a8b 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -46,6 +46,8 @@
#include "utils/syscache.h"
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
+static List *merge_subpublications(HeapTuple tuple, TupleDesc tupledesc,
+ List *publications, bool addpub);
/*
* Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
@@ -857,6 +859,51 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
break;
}
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ {
+ bool copy_data;
+ bool refresh;
+ List *publications = NIL;
+
+ publications = merge_subpublications(tup, RelationGetDescr(rel),
+ stmt->publication,
+ stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION);
+
+ parse_subscription_options(stmt->options,
+ NULL, /* no "connect" */
+ NULL, NULL, /* no "enabled" */
+ NULL, /* no "create_slot" */
+ NULL, NULL, /* no "slot_name" */
+ ©_data,
+ NULL, /* no "synchronous_commit" */
+ &refresh,
+ NULL, NULL, /* no "binary" */
+ NULL, NULL); /* no "streaming" */
+ values[Anum_pg_subscription_subpublications - 1] =
+ publicationListToArray(publications);
+ replaces[Anum_pg_subscription_subpublications - 1] = true;
+
+ update_tuple = true;
+
+ /* Refresh if user asked us to. */
+ if (refresh)
+ {
+ if (!sub->enabled)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
+
+ /* Make sure refresh sees the new list of publications. */
+ sub->publications = publications;
+
+ AlterSubscription_refresh(sub, copy_data);
+ }
+
+ break;
+ }
+
case ALTER_SUBSCRIPTION_REFRESH:
{
bool copy_data;
@@ -1278,3 +1325,75 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
return tablelist;
}
+
+/*
+ * merge current subpublications and user specified by add/drop publications.
+ *
+ * If addpub == true, we will add the list of publications into current
+ * subpublications. Otherwise, we will delete the list of publications from
+ * current subpublications.
+ */
+static List *
+merge_subpublications(HeapTuple tuple, TupleDesc tupledesc,
+ List *publications, bool addpub)
+{
+ int i;
+ int noldpublications;
+ Datum *oldpublications;
+ bool nulls[Natts_pg_subscription];
+ Datum values[Natts_pg_subscription];
+ List *merged = NIL;
+ ListCell *lc;
+ ArrayType *array;
+
+ /* deconstruct the subpublications */
+ heap_deform_tuple(tuple, tupledesc, values, nulls);
+ array = DatumGetArrayTypeP(values[Anum_pg_subscription_subpublications - 1]);
+ deconstruct_array(array, TEXTOID, -1, false, TYPALIGN_INT,
+ &oldpublications, NULL, &noldpublications);
+
+ for (i = 0; i < noldpublications; i++)
+ merged = lappend(merged, makeString(TextDatumGetCString((oldpublications[i]))));
+
+ foreach(lc, publications)
+ {
+ char *name = strVal(lfirst(lc));
+ ListCell *cell = NULL;
+
+ foreach(cell, merged)
+ {
+ char *subpub = strVal(lfirst(cell));
+
+ if (strcmp(name, subpub) == 0)
+ {
+ if (addpub)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("publication name \"%s\" is already in subscription",
+ name)));
+ }
+ else
+ {
+ merged = list_delete_cell(merged, cell);
+ break;
+ }
+ }
+ }
+
+ if (addpub)
+ merged = lappend(merged, makeString(name));
+ else if (cell == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("publication name \"%s\" do not in subscription",
+ name)));
+ }
+
+ if (merged == NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("subscription must contain at least one publication")));
+
+ return merged;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7574d545e0..d20e513518 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9615,6 +9615,26 @@ AlterSubscriptionStmt:
n->options = $7;
$$ = (Node *)n;
}
+ | ALTER SUBSCRIPTION name ADD_P PUBLICATION name_list opt_definition
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+ n->kind = ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+ n->subname = $3;
+ n->publication = $6;
+ n->options = $7;
+ $$ = (Node *)n;
+ }
+ | ALTER SUBSCRIPTION name DROP PUBLICATION name_list opt_definition
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+ n->kind = ALTER_SUBSCRIPTION_DROP_PUBLICATION;
+ n->subname = $3;
+ n->publication = $6;
+ n->options = $7;
+ $$ = (Node *)n;
+ }
| ALTER SUBSCRIPTION name ENABLE_P
{
AlterSubscriptionStmt *n =
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index dc2bb40926..9148ca9888 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3553,6 +3553,8 @@ typedef enum AlterSubscriptionType
ALTER_SUBSCRIPTION_OPTIONS,
ALTER_SUBSCRIPTION_CONNECTION,
ALTER_SUBSCRIPTION_PUBLICATION,
+ ALTER_SUBSCRIPTION_ADD_PUBLICATION,
+ ALTER_SUBSCRIPTION_DROP_PUBLICATION,
ALTER_SUBSCRIPTION_REFRESH,
ALTER_SUBSCRIPTION_ENABLED
} AlterSubscriptionType;
--
2.30.0
>From 870ec0b1c208b9cb150ce5268d5607c892ad3662 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 17:43:11 +0800
Subject: [PATCH v2 2/4] Test ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
---
src/test/regress/expected/subscription.out | 27 ++++++++++++++++++++++
src/test/regress/sql/subscription.sql | 19 +++++++++++++++
2 files changed, 46 insertions(+)
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 2fa9bce66a..f0412220bc 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -200,6 +200,33 @@ ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
regress_testsub | regress_subscription_user | f | {testpub} | f | f | off | dbname=regress_doesnotexist
(1 row)
+-- fail - publication already exists
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION testpub WITH (refresh = false);
+ERROR: publication name "testpub" is already in subscription
+-- ok - add two publications into subscription
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION testpub1, testpub2 WITH (refresh = false);
+\dRs+
+ List of subscriptions
+ Name | Owner | Enabled | Publication | Binary | Streaming | Synchronous commit | Conninfo
+-----------------+---------------------------+---------+-----------------------------+--------+-----------+--------------------+-----------------------------
+ regress_testsub | regress_subscription_user | f | {testpub,testpub1,testpub2} | f | f | off | dbname=regress_doesnotexist
+(1 row)
+
+-- fail - all publications are deleted
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub, testpub1, testpub2 WITH (refresh = false);
+ERROR: subscription must contain at least one publication
+-- fail - the deleted publications do not in subscription
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
+ERROR: publication name "testpub3" do not in subscription
+-- ok - delete publications
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub2 WITH (refresh = false);
+\dRs+
+ List of subscriptions
+ Name | Owner | Enabled | Publication | Binary | Streaming | Synchronous commit | Conninfo
+-----------------+---------------------------+---------+-------------+--------+-----------+--------------------+-----------------------------
+ regress_testsub | regress_subscription_user | f | {testpub} | f | f | off | dbname=regress_doesnotexist
+(1 row)
+
DROP SUBSCRIPTION regress_testsub;
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 14fa0b247e..ffb93f084d 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -145,6 +145,25 @@ ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
\dRs+
+-- fail - publication already exists
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION testpub WITH (refresh = false);
+
+-- ok - add two publications into subscription
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION testpub1, testpub2 WITH (refresh = false);
+
+\dRs+
+
+-- fail - all publications are deleted
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub, testpub1, testpub2 WITH (refresh = false);
+
+-- fail - the deleted publications do not in subscription
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
+
+-- ok - delete publications
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub2 WITH (refresh = false);
+
+\dRs+
+
DROP SUBSCRIPTION regress_testsub;
RESET SESSION AUTHORIZATION;
--
2.30.0
>From c5366ed1840259c5092e33bfe6570227d3858537 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 17:54:44 +0800
Subject: [PATCH v2 3/4] Add documentation for ALTER SUBSCRIPTION...ADD/DROP
PUBLICATION
---
doc/src/sgml/ref/alter_subscription.sgml | 68 ++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index db5e59f707..97c427e0f4 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -23,6 +23,8 @@ PostgreSQL documentation
<synopsis>
ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> CONNECTION '<replaceable>conninfo</replaceable>'
ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> SET 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> 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>] [, ... ] ) ]
ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> REFRESH PUBLICATION [ WITH ( <replaceable class="parameter">refresh_option</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) ]
ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> ENABLE
ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> DISABLE
@@ -107,6 +109,72 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>ADD PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
+ <listitem>
+ <para>
+ Add list of publications to subscription. See
+ <xref linkend="sql-createsubscription"/> for more information.
+ By default this command will also act like <literal>REFRESH
+ PUBLICATION</literal>.
+ </para>
+
+ <para>
+ <replaceable>set_publication_option</replaceable> specifies additional
+ options for this operation. The supported options are:
+
+ <variablelist>
+ <varlistentry>
+ <term><literal>refresh</literal> (<type>boolean</type>)</term>
+ <listitem>
+ <para>
+ When false, the command will not try to refresh table information.
+ <literal>REFRESH PUBLICATION</literal> should then be executed separately.
+ The default is <literal>true</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Additionally, refresh options as described
+ under <literal>REFRESH PUBLICATION</literal> may be specified.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>DROP PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
+ <listitem>
+ <para>
+ Drop list of publications from subscription. See
+ <xref linkend="sql-createsubscription"/> for more information.
+ By default this command will also act like <literal>REFRESH
+ PUBLICATION</literal>.
+ </para>
+
+ <para>
+ <replaceable>set_publication_option</replaceable> specifies additional
+ options for this operation. The supported options are:
+
+ <variablelist>
+ <varlistentry>
+ <term><literal>refresh</literal> (<type>boolean</type>)</term>
+ <listitem>
+ <para>
+ When false, the command will not try to refresh table information.
+ <literal>REFRESH PUBLICATION</literal> should then be executed separately.
+ The default is <literal>true</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Additionally, refresh options as described
+ under <literal>REFRESH PUBLICATION</literal> may be specified.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>REFRESH PUBLICATION</literal></term>
<listitem>
--
2.30.0
>From 25685b84e624bfffa4a57f8e01f88b3314029037 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 18:25:10 +0800
Subject: [PATCH v2 4/4] Add tab-complete for ALTER SUBSCRIPTION...ADD/DROP
---
src/bin/psql/tab-complete.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 17f7265038..dd178c48e2 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1634,7 +1634,8 @@ psql_completion(const char *text, int start, int end)
/* ALTER SUBSCRIPTION <name> */
else if (Matches("ALTER", "SUBSCRIPTION", MatchAny))
COMPLETE_WITH("CONNECTION", "ENABLE", "DISABLE", "OWNER TO",
- "RENAME TO", "REFRESH PUBLICATION", "SET");
+ "RENAME TO", "REFRESH PUBLICATION", "SET",
+ "ADD PUBLICATION", "DROP PUBLICATION");
/* ALTER SUBSCRIPTION <name> REFRESH PUBLICATION */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("REFRESH", "PUBLICATION"))
@@ -1658,6 +1659,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
+ /* ALTER SUBSCRIPTION <name> ADD PUBLICATION <name> */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("ADD", "PUBLICATION", MatchAny))
+ COMPLETE_WITH("WITH (");
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny))
+ COMPLETE_WITH("WITH (");
/* ALTER SUBSCRIPTION <name> SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("SET", "PUBLICATION", MatchAny, "WITH", "("))
--
2.30.0
