On Fri, 05 Feb 2021 at 17:50, Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
> On Wed, Feb 3, 2021 at 2:02 PM japin <japi...@hotmail.com> wrote:
>> On Wed, 03 Feb 2021 at 13:15, Bharath Rupireddy
>> <bharath.rupireddyforpostg...@gmail.com> wrote:
>> > On Thu, Jan 28, 2021 at 10:07 AM japin <japi...@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.
>From 9009442c1605ec9e6d3f75613bf4915888a12f6f Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Fri, 5 Feb 2021 20:49:40 +0800
Subject: [PATCH v4 1/5] Export textarray_to_stringlist
---
src/backend/catalog/pg_subscription.c | 3 +--
src/include/catalog/pg_subscription.h | 2 ++
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 44cb285b68..66d057bed8 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -33,7 +33,6 @@
#include "utils/rel.h"
#include "utils/syscache.h"
-static List *textarray_to_stringlist(ArrayType *textarray);
/*
* Fetch the subscription from the syscache.
@@ -209,7 +208,7 @@ get_subscription_name(Oid subid, bool missing_ok)
*
* Note: the resulting list of strings is pallocated here.
*/
-static List *
+List *
textarray_to_stringlist(ArrayType *textarray)
{
Datum *elems;
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a5d6efdf20..3fc558c7c5 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -21,6 +21,7 @@
#include "catalog/pg_subscription_d.h"
#include "nodes/pg_list.h"
+#include "utils/array.h"
/* ----------------
* pg_subscription definition. cpp turns this into
@@ -101,6 +102,7 @@ extern Subscription *GetSubscription(Oid subid, bool missing_ok);
extern void FreeSubscription(Subscription *sub);
extern Oid get_subscription_oid(const char *subname, bool missing_ok);
extern char *get_subscription_name(Oid subid, bool missing_ok);
+extern List *textarray_to_stringlist(ArrayType *textarray);
extern int CountDBSubscriptions(Oid dbid);
--
2.30.0
>From 276616ad59ede034b278eded767ee40b266eaa00 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Fri, 5 Feb 2021 20:59:30 +0800
Subject: [PATCH v4 2/5] Introduce a new syntax to add/drop publications
At present, if we want to update publications in subscription, we can
use SET PUBLICATION, however, it requires supply all publications that
exists and the new publications if we want to add new publications, it's
inconvenient. The new syntax only supply the new publications. When
the refresh is true, it only refresh the new publications.
---
src/backend/commands/subscriptioncmds.c | 113 ++++++++++++++++++++++++
src/backend/parser/gram.y | 20 +++++
src/include/nodes/parsenodes.h | 2 +
3 files changed, 135 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 082f7855b8..7f296f76a8 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, List *publications,
+ bool addpub);
/*
* Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
@@ -857,6 +859,50 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
break;
}
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ {
+ bool copy_data = false;
+ bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
+ bool refresh;
+ List *publist = NIL;
+
+ publist = merge_subpublications(tup, stmt->publication, isadd);
+
+ parse_subscription_options(stmt->options,
+ NULL, /* no "connect" */
+ NULL, NULL, /* no "enabled" */
+ NULL, /* no "create_slot" */
+ NULL, NULL, /* no "slot_name" */
+ isadd ? ©_data : NULL, /* for drop, no "copy_data" */
+ NULL, /* no "synchronous_commit" */
+ &refresh,
+ NULL, NULL, /* no "binary" */
+ NULL, NULL); /* no "streaming" */
+ values[Anum_pg_subscription_subpublications - 1] =
+ publicationListToArray(publist);
+ 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).")));
+
+ /* Only refresh the added/dropped list of publications. */
+ sub->publications = stmt->publication;
+
+ AlterSubscription_refresh(sub, copy_data);
+ }
+
+ break;
+ }
+
case ALTER_SUBSCRIPTION_REFRESH:
{
bool copy_data;
@@ -1278,3 +1324,70 @@ fetch_table_list(WalReceiverConn *wrconn, List *publications)
return tablelist;
}
+
+/*
+ * Merge current subscription's publications and user specified publications
+ * by ADD/DROP PUBLICATIONS.
+ *
+ * If isadd == true, we will add the list of publications into current
+ * subscription's publications. Otherwise, we will delete the list of
+ * publications from current subscription's publications.
+ */
+static List *
+merge_subpublications(HeapTuple tuple, List *newpublist, bool isadd)
+{
+ Datum datum;
+ bool isnull;
+ List *publist = NIL;
+ ListCell *lc;
+
+ /* Get publications */
+ datum = SysCacheGetAttr(SUBSCRIPTIONOID,
+ tuple,
+ Anum_pg_subscription_subpublications,
+ &isnull);
+ Assert(!isnull);
+ publist = textarray_to_stringlist(DatumGetArrayTypeP(datum));
+
+ foreach(lc, newpublist)
+ {
+ char *name = strVal(lfirst(lc));
+ ListCell *cell = NULL;
+
+ foreach(cell, publist)
+ {
+ char *pubname = strVal(lfirst(cell));
+
+ if (strcmp(name, pubname) == 0)
+ {
+ if (isadd)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("publication name \"%s\" is already in subscription",
+ name)));
+ }
+ else
+ {
+ publist = list_delete_cell(publist, cell);
+ break;
+ }
+ }
+ }
+
+ if (isadd)
+ publist = lappend(publist, makeString(name));
+ else if (cell == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("publication name \"%s\" does not exists in subscription",
+ name)));
+ }
+
+ if (publist == NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("subscription must contain at least one publication")));
+
+ return publist;
+}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dd72a9fc3c..77b28fc0cf 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9619,6 +9619,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 236832a2ca..8021f66595 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3581,6 +3581,8 @@ typedef enum AlterSubscriptionType
ALTER_SUBSCRIPTION_CONNECTION,
ALTER_SUBSCRIPTION_PUBLICATION,
ALTER_SUBSCRIPTION_REFRESH,
+ ALTER_SUBSCRIPTION_ADD_PUBLICATION,
+ ALTER_SUBSCRIPTION_DROP_PUBLICATION,
ALTER_SUBSCRIPTION_ENABLED
} AlterSubscriptionType;
--
2.30.0
>From 8ba22fb50e12baad14b17744ce05d019231fd46a Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 17:43:11 +0800
Subject: [PATCH v4 3/5] Test ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
---
src/test/regress/expected/subscription.out | 30 ++++++++++++++++++++++
src/test/regress/sql/subscription.sql | 22 ++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 2fa9bce66a..4856504045 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -200,6 +200,36 @@ 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" does not exists in subscription
+-- fail - do not support copy_data option
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1 WITH (refresh = false, copy_data = true);
+ERROR: unrecognized subscription parameter: "copy_data"
+-- 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..a0178cfb7e 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -145,6 +145,28 @@ 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);
+
+-- fail - do not support copy_data option
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1 WITH (refresh = false, copy_data = true);
+
+-- 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 7aae8ac36999cc7823f0e68f1fa596af56e4f6b2 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 17:54:44 +0800
Subject: [PATCH v4 4/5] Add documentation for ALTER SUBSCRIPTION...ADD/DROP
PUBLICATION
---
doc/src/sgml/ref/alter_subscription.sgml | 65 ++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index db5e59f707..e6e81ce7a3 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,69 @@ 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>, except it only affect on added publications.
+ </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>, except it only affect on dropped publications.
+ </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>
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>REFRESH PUBLICATION</literal></term>
<listitem>
--
2.30.0
>From 40f1a5ec677dbbaea82a5a384a35d220dd1f7683 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 26 Jan 2021 18:25:10 +0800
Subject: [PATCH v4 5/5] 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 5f0e775fd3..075b83b28f 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
