On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Wed, May 12, 2021 at 9:55 PM vignesh C <vignes...@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
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
