alter subscription drop publication fixes
Hi,
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".
merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.
Attached a patch which contains the fixes for the same.
Thoughts?
Regards,
Vignesh
Attachments:
v1-0001-Fixes-in-alter-subscription-drop-publication.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fixes-in-alter-subscription-drop-publication.patchDownload
From 516f7b933f06b64ebb21e3f55bdd223703879a3a Mon Sep 17 00:00:00 2001
From: vignesh <vignesh21@gmail.com>
Date: Mon, 10 May 2021 12:15:28 +0530
Subject: [PATCH v1] Fixes in alter subscription drop publication.
Fixes in alter subscription drop publication.
---
src/backend/commands/subscriptioncmds.c | 6 +++---
src/bin/psql/tab-complete.c | 9 ++++++---
src/test/regress/expected/subscription.out | 2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bbb2f5d029..55ce24e613 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("dropping specified publication will result in subscription without any publication, this is not supported")));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d917987fd5..8ec78dc734 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
- /* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
COMPLETE_WITH("copy_data", "refresh");
-
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+ COMPLETE_WITH("refresh");
/* ALTER SCHEMA <name> */
else if (Matches("ALTER", "SCHEMA", MatchAny))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..c71e86a797 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- 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
+ERROR: dropping specified publication will result in subscription without any publication, this is not supported
-- fail - publication does not exist in subscription
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
ERROR: publication "testpub3" is not in subscription "regress_testsub"
--
2.25.1
On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@gmail.com> wrote:
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.
Thanks!
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.
+1. You may want to also change set_publication_option(to something
like drop_pulication_option with only refresh option) for the drop in
the docs? Because "Additionally, refresh options as described under
REFRESH PUBLICATION may be specified." doesn't make sense.
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".
-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.
merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.
+1. That was a miss in the original feature.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Thu, 13 May 2021 at 00:45, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@gmail.com> wrote:
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.Thanks!
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.+1. You may want to also change set_publication_option(to something
like drop_pulication_option with only refresh option) for the drop in
the docs? Because "Additionally, refresh options as described under
REFRESH PUBLICATION may be specified." doesn't make sense.
+1. Make sense to remove the unsupported options for tab-complete.
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.
Agree with Bharath. We can use a detail message. How about?
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("subscription must contain at least one publication"),
errdetail("Dropping all the publications from a subscription is not supported")));
merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.+1. That was a miss in the original feature.
+1.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Thu, May 13, 2021 at 8:45 AM Japin Li <japinli@hotmail.com> wrote:
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.Agree with Bharath. We can use a detail message. How about?
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("subscription must contain at least one publication"),
errdetail("Dropping all the publications from a subscription is not supported")));
Or how about just errmsg("cannot drop all the publications of the
subscriber \"%s\"", subname) without any error detail?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Thu, May 13, 2021 at 9:36 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Thu, May 13, 2021 at 8:45 AM Japin Li <japinli@hotmail.com> wrote:
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.Agree with Bharath. We can use a detail message. How about?
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("subscription must contain at least one publication"),
errdetail("Dropping all the publications from a subscription is not supported")));Or how about just errmsg("cannot drop all the publications of the
subscriber \"%s\"", subname) without any error detail?
IMHO, this message without errdetail looks much better.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@gmail.com> wrote:
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.Thanks!
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.+1. You may want to also change set_publication_option(to something
like drop_pulication_option with only refresh option) for the drop in
the docs? Because "Additionally, refresh options as described under
REFRESH PUBLICATION may be specified." doesn't make sense.Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.
Modified the error message to "errmsg("cannot drop all the
publications of the subscriber \"%s\"", subname)".
I have separated the Drop publication documentation contents. There
are some duplicate contents but the readability is slightly better.
Thoughts?
merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.+1. That was a miss in the original feature.
Attached patch has the changes for the same.
Regards,
Vignesh
Attachments:
v2-0001-Fixes-in-alter-subscription-drop-publication.patchapplication/x-patch; name=v2-0001-Fixes-in-alter-subscription-drop-publication.patchDownload
From b73e99bc2001c23f64fb3b4f220e1a4439614dd6 Mon Sep 17 00:00:00 2001
From: vignesh <vignesh21@gmail.com>
Date: Mon, 10 May 2021 12:15:28 +0530
Subject: [PATCH v2] Fixes in alter subscription drop publication.
Fixes in alter subscription drop publication.
---
doc/src/sgml/ref/alter_subscription.sgml | 37 ++++++++++++++++++----
src/backend/commands/subscriptioncmds.c | 6 ++--
src/bin/psql/tab-complete.c | 9 ++++--
src/test/regress/expected/subscription.out | 2 +-
4 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..2bf836ed5a 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -24,7 +24,7 @@ PostgreSQL documentation
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> DROP PUBLICATION <replaceable class="parameter">publication_name</replaceable> [, ...] [ WITH ( refresh [= <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
@@ -94,21 +94,46 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>DROP PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
+ <listitem>
+ <para>
+ Removes the specified publications from the list of publications. See
+ <xref linkend="sql-createsubscription"/> for more information. By
+ default the dropped publications are refreshed.
+ </para>
+
+ <para>
+ The following option is supported:
+
+ <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>SET PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
<term><literal>ADD PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
- <term><literal>DROP PUBLICATION <replaceable class="parameter">publication_name</replaceable></literal></term>
<listitem>
<para>
Changes the list of subscribed publications. <literal>SET</literal>
replaces the entire list of publications with a new list,
- <literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
+ <literal>ADD</literal> adds additional publications from the list of
publications. See <xref linkend="sql-createsubscription"/> for more
information. By default, this command will also act like
<literal>REFRESH PUBLICATION</literal>, except that in case of
- <literal>ADD</literal> or <literal>DROP</literal>, only the added or
- dropped publications are refreshed.
+ <literal>ADD</literal>, only the added publications are refreshed.
</para>
<para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..e93fee6b99 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("cannot drop all the publications of the subscriber \"%s\"", subname)));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..2595941408 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
- /* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
COMPLETE_WITH("copy_data", "refresh");
-
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+ COMPLETE_WITH("refresh");
/* ALTER SCHEMA <name> */
else if (Matches("ALTER", "SCHEMA", MatchAny))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..76de317830 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- 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
+ERROR: cannot drop all the publications of the subscriber "regress_testsub"
-- fail - publication does not exist in subscription
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
ERROR: publication "testpub3" is not in subscription "regress_testsub"
--
2.25.1
On Thu, May 13, 2021 at 7:43 PM vignesh C <vignesh21@gmail.com> wrote:
I have separated the Drop publication documentation contents. There
are some duplicate contents but the readability is slightly better.
Thoughts?
-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>
DROP PUBLICATION <replaceable
class="parameter">publication_name</replaceable> [, ...] [ WITH (
refresh [= <replaceable class="parameter">value</replaceable>] ) ]IMO, let's not list the "refresh" option directly here. If we don't
want to add a new list of operations "drop_publication_opition", you
could just mention a note "Except for DROP PUBLICATION, the refresh
options as described under REFRESH PUBLICATION may be specified." or
"Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except for DROP PUBLICATION."
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Thu, 13 May 2021 at 22:13, vignesh C <vignesh21@gmail.com> wrote:
On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@gmail.com> wrote:
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.Thanks!
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.+1. You may want to also change set_publication_option(to something
like drop_pulication_option with only refresh option) for the drop in
the docs? Because "Additionally, refresh options as described under
REFRESH PUBLICATION may be specified." doesn't make sense.Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.Modified the error message to "errmsg("cannot drop all the
publications of the subscriber \"%s\"", subname)".
I have separated the Drop publication documentation contents. There
are some duplicate contents but the readability is slightly better.
Thoughts?merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.+1. That was a miss in the original feature.
Attached patch has the changes for the same.
Thanks for updating the patch. I have a little comments for the new patch.
- <literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
+ <literal>ADD</literal> adds additional publications from the list ofI think, we should change the word 'from' to 'to'.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Thu, May 13, 2021 at 8:13 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Thu, May 13, 2021 at 7:43 PM vignesh C <vignesh21@gmail.com> wrote:
I have separated the Drop publication documentation contents. There
are some duplicate contents but the readability is slightly better.
Thoughts?-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> DROP PUBLICATION <replaceable class="parameter">publication_name</replaceable> [, ...] [ WITH ( refresh [= <replaceable class="parameter">value</replaceable>] ) ]IMO, let's not list the "refresh" option directly here. If we don't
want to add a new list of operations "drop_publication_opition", you
could just mention a note "Except for DROP PUBLICATION, the refresh
options as described under REFRESH PUBLICATION may be specified." or
"Additionally, refresh options as described under REFRESH PUBLICATION
may be specified, except for DROP PUBLICATION."
Thanks for the comment, the attached v3 patch has the changes for the
same. I also made another change to change set_publication_option to
publication_option as it is common for SET/ADD & DROP.
Regards,
Vignesh
Attachments:
v3-0001-Fixes-in-alter-subscription-drop-publication.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Fixes-in-alter-subscription-drop-publication.patchDownload
From 3b477122f909d5e1df35c32a1775665bc92ec86b Mon Sep 17 00:00:00 2001
From: vignesh <vignesh21@gmail.com>
Date: Fri, 14 May 2021 19:30:40 +0530
Subject: [PATCH v3] Fixes in alter subscription drop publication.
Fixes in alter subscription drop publication.
---
doc/src/sgml/ref/alter_subscription.sgml | 13 +++++++------
src/backend/commands/subscriptioncmds.c | 6 +++---
src/bin/psql/tab-complete.c | 9 ++++++---
src/test/regress/expected/subscription.out | 2 +-
4 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..faf785760c 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,9 +22,9 @@ PostgreSQL documentation
<refsynopsisdiv>
<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> SET PUBLICATION <replaceable class="parameter">publication_name</replaceable> [, ...] [ WITH ( <replaceable class="parameter">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">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">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
@@ -103,7 +103,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
Changes the list of subscribed publications. <literal>SET</literal>
replaces the entire list of publications with a new list,
<literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
+ <literal>DROP</literal> removes publications to/from the list of
publications. See <xref linkend="sql-createsubscription"/> for more
information. By default, this command will also act like
<literal>REFRESH PUBLICATION</literal>, except that in case of
@@ -112,7 +112,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</para>
<para>
- <replaceable>set_publication_option</replaceable> specifies additional
+ <replaceable>publication_option</replaceable> specifies additional
options for this operation. The supported options are:
<variablelist>
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</variablelist>
Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ under <literal>REFRESH PUBLICATION</literal> may be specified, except for
+ <literal>DROP PUBLICATION</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..e93fee6b99 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("cannot drop all the publications of the subscriber \"%s\"", subname)));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..2595941408 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
- /* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
COMPLETE_WITH("copy_data", "refresh");
-
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+ COMPLETE_WITH("refresh");
/* ALTER SCHEMA <name> */
else if (Matches("ALTER", "SCHEMA", MatchAny))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..76de317830 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- 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
+ERROR: cannot drop all the publications of the subscriber "regress_testsub"
-- fail - publication does not exist in subscription
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
ERROR: publication "testpub3" is not in subscription "regress_testsub"
--
2.25.1
On Fri, May 14, 2021 at 7:41 AM Japin Li <japinli@hotmail.com> wrote:
On Thu, 13 May 2021 at 22:13, vignesh C <vignesh21@gmail.com> wrote:
On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:On Wed, May 12, 2021 at 9:55 PM vignesh C <vignesh21@gmail.com> wrote:
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.Thanks!
Alter subscription drop publication does not support copy_data
option,
that needs to be removed from tab completion.
+1. You may want to also change set_publication_option(to something
like drop_pulication_option with only refresh option) for the drop in
the docs? Because "Additionally, refresh options as described under
REFRESH PUBLICATION may be specified." doesn't make sense.Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".-1 for that long message. The intention of that error was to throw an
error if all the publications of a subscription are dropped. If that's
so confusing, then you could just let the error message be
"subscription must contain at least one publication", add an error
detail "Subscription without any publication is not allowed to
exist/is not supported." or "Removing/Dropping all the publications
from a subscription is not allowed/supported." or some other better
wording.Modified the error message to "errmsg("cannot drop all the
publications of the subscriber \"%s\"", subname)".
I have separated the Drop publication documentation contents. There
are some duplicate contents but the readability is slightly better.
Thoughts?merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.+1. That was a miss in the original feature.
Attached patch has the changes for the same.
Thanks for updating the patch. I have a little comments for the new patch.
- <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>ADD</literal> adds additional publications from the list
of
I think, we should change the word 'from' to 'to'.
I have changed it to:
<literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
+ <literal>DROP</literal> removes publications to/from the list ofThe changes for the same are shared in v3 patch at [1]/messages/by-id/CALDaNm3svMg+hMA9GsJsUQ75HXtpjpAh2gk=8yZfgAnA9BMsnA@mail.gmail.com.
[1]: /messages/by-id/CALDaNm3svMg+hMA9GsJsUQ75HXtpjpAh2gk=8yZfgAnA9BMsnA@mail.gmail.com
/messages/by-id/CALDaNm3svMg+hMA9GsJsUQ75HXtpjpAh2gk=8yZfgAnA9BMsnA@mail.gmail.com
Regards,
Vignesh
On Fri, May 14, 2021 at 7:58 PM vignesh C <vignesh21@gmail.com> wrote:
I have changed it to: <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>DROP</literal> removes publications to/from the list of
How about "Publications are added to or dropped from the existing list
of publications by <literal>ADD</literal> or <literal>DROP</literal>
respectively." ?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
On Fri, May 14, 2021 at 7:58 PM vignesh C <vignesh21@gmail.com> wrote:
I have changed it to: <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>DROP</literal> removes publications to/from the list of
How about "Publications are added to or dropped from the existing list
of publications by <literal>ADD</literal> or <literal>DROP</literal>
respectively." ?
We generally prefer to use the active voice, so I don't think
restructuring the sentence that way is an improvement. The quoted
bit would be better left alone entirely. Or maybe split it into
two sentences, but keep the active voice.
regards, tom lane
On Fri, May 14, 2021 at 8:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
On Fri, May 14, 2021 at 7:58 PM vignesh C <vignesh21@gmail.com> wrote:
I have changed it to: <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>DROP</literal> removes publications to/from the list ofHow about "Publications are added to or dropped from the existing list
of publications by <literal>ADD</literal> or <literal>DROP</literal>
respectively." ?We generally prefer to use the active voice, so I don't think
restructuring the sentence that way is an improvement. The quoted
bit would be better left alone entirely. Or maybe split it into
two sentences, but keep the active voice.
I felt changing it to the below was better:
SET replaces the entire list of publications with a new list, ADD adds
additional publications to the list of publications and DROP removes
the publications from the list of publications.
Attached patch has the change for the same.
Thoughts?
Regards,
Vignesh
Attachments:
v4-0001-Fixes-in-alter-subscription-drop-publication.patchtext/x-patch; charset=US-ASCII; name=v4-0001-Fixes-in-alter-subscription-drop-publication.patchDownload
From 4f887b0b3f338f55d186fa059cfdc255b9783263 Mon Sep 17 00:00:00 2001
From: vignesh <vignesh21@gmail.com>
Date: Fri, 14 May 2021 19:30:40 +0530
Subject: [PATCH v4] Fixes in alter subscription drop publication.
Fixes in alter subscription drop publication.
---
doc/src/sgml/ref/alter_subscription.sgml | 19 ++++++++++---------
src/backend/commands/subscriptioncmds.c | 6 +++---
src/bin/psql/tab-complete.c | 9 ++++++---
src/test/regress/expected/subscription.out | 2 +-
4 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..6f5a4268e5 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,9 +22,9 @@ PostgreSQL documentation
<refsynopsisdiv>
<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> SET PUBLICATION <replaceable class="parameter">publication_name</replaceable> [, ...] [ WITH ( <replaceable class="parameter">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">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">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
@@ -102,17 +102,17 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
Changes the list of subscribed publications. <literal>SET</literal>
replaces the entire list of publications with a new list,
- <literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
- publications. See <xref linkend="sql-createsubscription"/> for more
- information. By default, this command will also act like
+ <literal>ADD</literal> adds additional publications to the list of
+ publications and <literal>DROP</literal> removes the publications from
+ the list of publications. See <xref linkend="sql-createsubscription"/>
+ for more information. By default, this command will also act like
<literal>REFRESH PUBLICATION</literal>, except that in case of
<literal>ADD</literal> or <literal>DROP</literal>, only the added or
dropped publications are refreshed.
</para>
<para>
- <replaceable>set_publication_option</replaceable> specifies additional
+ <replaceable>publication_option</replaceable> specifies additional
options for this operation. The supported options are:
<variablelist>
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</variablelist>
Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ under <literal>REFRESH PUBLICATION</literal> may be specified, except for
+ <literal>DROP PUBLICATION</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..e93fee6b99 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("cannot drop all the publications of the subscriber \"%s\"", subname)));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..2595941408 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
- /* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
COMPLETE_WITH("copy_data", "refresh");
-
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+ COMPLETE_WITH("refresh");
/* ALTER SCHEMA <name> */
else if (Matches("ALTER", "SCHEMA", MatchAny))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..76de317830 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- 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
+ERROR: cannot drop all the publications of the subscriber "regress_testsub"
-- fail - publication does not exist in subscription
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
ERROR: publication "testpub3" is not in subscription "regress_testsub"
--
2.25.1
On Fri, May 14, 2021 at 11:02 PM vignesh C <vignesh21@gmail.com> wrote:
On Fri, May 14, 2021 at 8:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
On Fri, May 14, 2021 at 7:58 PM vignesh C <vignesh21@gmail.com> wrote:
I have changed it to: <literal>ADD</literal> adds additional publications, - <literal>DROP</literal> removes publications from the list of + <literal>DROP</literal> removes publications to/from the list ofHow about "Publications are added to or dropped from the existing list
of publications by <literal>ADD</literal> or <literal>DROP</literal>
respectively." ?We generally prefer to use the active voice, so I don't think
restructuring the sentence that way is an improvement. The quoted
bit would be better left alone entirely. Or maybe split it into
two sentences, but keep the active voice.I felt changing it to the below was better:
SET replaces the entire list of publications with a new list, ADD adds
additional publications to the list of publications and DROP removes
the publications from the list of publications.Attached patch has the change for the same.
Thoughts?
Thanks Vignesh, the patch looks good to me and it works as expected
i.e. doesn't show up the copy_data option in the tab complete for the
alter subscription drop publication command. While on this, I observed
that the new function merge_publications and the error message crossed
the 80char limit, I adjusted that and added a commit message. Please
have a look, if that is okay, add an entry to the commit fest and pass
it on to the committer as I have no further comments.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v5-0001-Fixes-in-ALTER-SUBSCRIPTION-DROP-PUBLICATION-code.patchapplication/octet-stream; name=v5-0001-Fixes-in-ALTER-SUBSCRIPTION-DROP-PUBLICATION-code.patchDownload
From 9beda5d8a3189d0c189434eac6e5572d8b2f1eac Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Sat, 15 May 2021 14:49:22 +0530
Subject: [PATCH v5] Fixes in ALTER SUBSCRIPTION DROP PUBLICATION code
ALTER SUBSCRIPTION DROP PUBLICATION does not actually support
copy_data option, so remove it from tab completion.
Also, reword the error message that is thrown when all the
publications from a subscription are specified to be dropped.
Also, made few doc and cosmetic adjustments.
Author: Vignesh C
Reviewed-by: Bharath Rupireddy, Japin Li, Tom Lane
---
doc/src/sgml/ref/alter_subscription.sgml | 20 ++++++++++----------
src/backend/commands/subscriptioncmds.c | 16 +++++++++++-----
src/bin/psql/tab-complete.c | 9 ++++++---
src/test/regress/expected/subscription.out | 2 +-
4 files changed, 28 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 367ac814f4..177bb372aa 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,9 +22,9 @@ PostgreSQL documentation
<refsynopsisdiv>
<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> SET PUBLICATION <replaceable class="parameter">publication_name</replaceable> [, ...] [ WITH ( <replaceable class="parameter">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">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">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
@@ -102,17 +102,17 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
Changes the list of subscribed publications. <literal>SET</literal>
replaces the entire list of publications with a new list,
- <literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list of
- publications. See <xref linkend="sql-createsubscription"/> for more
- information. By default, this command will also act like
+ <literal>ADD</literal> adds additional publications to the list of
+ publications and <literal>DROP</literal> removes the publications from
+ the list of publications. See <xref linkend="sql-createsubscription"/>
+ for more information. By default, this command will also act like
<literal>REFRESH PUBLICATION</literal>, except that in case of
<literal>ADD</literal> or <literal>DROP</literal>, only the added or
dropped publications are refreshed.
</para>
<para>
- <replaceable>set_publication_option</replaceable> specifies additional
+ <replaceable>publication_option</replaceable> specifies additional
options for this operation. The supported options are:
<variablelist>
@@ -128,8 +128,8 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
</varlistentry>
</variablelist>
- Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ Additionally, refresh options as described under <literal>REFRESH PUBLICATION</literal>
+ may be specified, except for <literal>DROP PUBLICATION</literal>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8aa6de1785..c9c03d8fa3 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -48,7 +48,8 @@
static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
static void check_duplicates_in_publist(List *publist, Datum *datums);
-static List *merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname);
+static List *merge_publications(List *oldpublist, List *newpublist,
+ bool addpub, const char *subname);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -951,8 +952,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +964,11 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications,
+ stmt->publication,
+ isadd,
+ stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1622,7 +1626,8 @@ check_duplicates_in_publist(List *publist, Datum *datums)
* subname is the subscription name, for error messages.
*/
static List *
-merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname)
+merge_publications(List *oldpublist, List *newpublist, bool addpub,
+ const char *subname)
{
ListCell *lc;
@@ -1671,7 +1676,8 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("cannot drop all the publications of the subscriber \"%s\"",
+ subname)));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 6598c5369a..2595941408 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
COMPLETE_WITH("WITH (");
- /* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
COMPLETE_WITH("copy_data", "refresh");
-
+ /* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
+ else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+ TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+ COMPLETE_WITH("refresh");
/* ALTER SCHEMA <name> */
else if (Matches("ALTER", "SCHEMA", MatchAny))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..76de317830 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- 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
+ERROR: cannot drop all the publications of the subscriber "regress_testsub"
-- fail - publication does not exist in subscription
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
ERROR: publication "testpub3" is not in subscription "regress_testsub"
--
2.25.1
On Sat, May 15, 2021 at 2:58 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:
On Fri, May 14, 2021 at 11:02 PM vignesh C <vignesh21@gmail.com> wrote:
On Fri, May 14, 2021 at 8:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
On Fri, May 14, 2021 at 7:58 PM vignesh C <vignesh21@gmail.com>
wrote:
I have changed it to:
<literal>ADD</literal> adds additional publications,
- <literal>DROP</literal> removes publications from the list
of
+ <literal>DROP</literal> removes publications to/from the
list of
How about "Publications are added to or dropped from the existing
list
of publications by <literal>ADD</literal> or
<literal>DROP</literal>
respectively." ?
We generally prefer to use the active voice, so I don't think
restructuring the sentence that way is an improvement. The quoted
bit would be better left alone entirely. Or maybe split it into
two sentences, but keep the active voice.I felt changing it to the below was better:
SET replaces the entire list of publications with a new list, ADD adds
additional publications to the list of publications and DROP removes
the publications from the list of publications.Attached patch has the change for the same.
Thoughts?Thanks Vignesh, the patch looks good to me and it works as expected
i.e. doesn't show up the copy_data option in the tab complete for the
alter subscription drop publication command. While on this, I observed
that the new function merge_publications and the error message crossed
the 80char limit, I adjusted that and added a commit message. Please
have a look, if that is okay, add an entry to the commit fest and pass
it on to the committer as I have no further comments.
Thanks Bharath, that looks good. I have added a commitfest entry at [1]https://commitfest.postgresql.org/33/3115/ and
marked it to Ready For Committer.
[1]: https://commitfest.postgresql.org/33/3115/
Regards,
Vignesh
On 15.05.21 15:15, vignesh C wrote:
Thanks Bharath, that looks good. I have added a commitfest entry at [1]
and marked it to Ready For Committer.
[1] - https://commitfest.postgresql.org/33/3115/
<https://commitfest.postgresql.org/33/3115/>
Committed.
I took out some of the code reformatting. We have pgindent for that,
and it didn't object to the existing formatting, so we don't need to
worry about that very much.
On Fri, Jun 25, 2021 at 1:30 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
On 15.05.21 15:15, vignesh C wrote:
Thanks Bharath, that looks good. I have added a commitfest entry at [1]
and marked it to Ready For Committer.
[1] - https://commitfest.postgresql.org/33/3115/
<https://commitfest.postgresql.org/33/3115/>Committed.
I took out some of the code reformatting. We have pgindent for that,
and it didn't object to the existing formatting, so we don't need to
worry about that very much.
Thanks for committing this.
Regards,
Vignesh