Re: alter subscription drop publication fixes

[vignesh C]

On Fri, Jun 25, 2021 at 1:30 PM Peter Eisentraut
 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/
> > 
>
> 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

Re: alter subscription drop publication fixes

[Peter Eisentraut]

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/ 



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.

Re: alter subscription drop publication fixes

[vignesh C]

On Sat, May 15, 2021 at 2:58 PM Bharath Rupireddy <
bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Fri, May 14, 2021 at 11:02 PM vignesh C  wrote:
> >
> > On Fri, May 14, 2021 at 8:56 PM Tom Lane  wrote:
> > >
> > > Bharath Rupireddy  writes:
> > > > On Fri, May 14, 2021 at 7:58 PM vignesh C 
wrote:
> > > >> I have changed it to:
> > > >> ADD adds additional publications,
> > > >> -  DROP removes publications from the list
of
> > > >> +  DROP removes publications to/from the
list of
> > >
> > > > How about "Publications are added to or dropped from the existing
list
> > > > of publications by ADD  or
DROP
> > > > 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] and
marked it to Ready For Committer.
[1] - https://commitfest.postgresql.org/33/3115/

Regards,
Vignesh

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Fri, May 14, 2021 at 11:02 PM vignesh C  wrote:
>
> On Fri, May 14, 2021 at 8:56 PM Tom Lane  wrote:
> >
> > Bharath Rupireddy  writes:
> > > On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
> > >> I have changed it to:
> > >> ADD adds additional publications,
> > >> -  DROP removes publications from the list of
> > >> +  DROP removes publications to/from the list of
> >
> > > How about "Publications are added to or dropped from the existing list
> > > of publications by ADD  or DROP
> > > 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


v5-0001-Fixes-in-ALTER-SUBSCRIPTION-DROP-PUBLICATION-code.patch
Description: Binary data

Re: alter subscription drop publication fixes

[vignesh C]

On Fri, May 14, 2021 at 8:56 PM Tom Lane  wrote:
>
> Bharath Rupireddy  writes:
> > On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
> >> I have changed it to:
> >> ADD adds additional publications,
> >> -  DROP removes publications from the list of
> >> +  DROP removes publications to/from the list of
>
> > How about "Publications are added to or dropped from the existing list
> > of publications by ADD  or DROP
> > 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
From 4f887b0b3f338f55d186fa059cfdc255b9783263 Mon Sep 17 00:00:00 2001
From: vignesh 
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
  
 
 ALTER SUBSCRIPTION name CONNECTION 'conninfo'
-ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -102,17 +102,17 @@ ALTER SUBSCRIPTION name RENAME TO <
  
   Changes the list of subscribed publications.  SET
   replaces the entire list of publications with a new list,
-  ADD adds additional publications,
-  DROP removes publications from the list of
-  publications.  See  for more
-  information.  By default, this command will also act like
+  ADD adds additional publications to the list of
+  publications and DROP removes the publications from
+  the list of publications.  See 
+  for more information.  By default, this command will also act like
   REFRESH PUBLICATION, except that in case of
   ADD or DROP, only the added or
   dropped publications are refreshed.
  
 
  
-  set_publication_option specifies additional
+  publication_option specifies additional
   options for this operation.  The supported options are:
 
   
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION name RENAME TO <
   
 
   Additionally, refresh options as described
-  under REFRESH PUBLICATION may be specified.
+  under REFRESH PUBLICATION may be specified, except for
+  DROP PUBLICATION.
  
 

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] =
 

Re: alter subscription drop publication fixes

[Tom Lane]

Bharath Rupireddy  writes:
> On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
>> I have changed it to:
>> ADD adds additional publications,
>> -  DROP removes publications from the list of
>> +  DROP removes publications to/from the list of

> How about "Publications are added to or dropped from the existing list
> of publications by ADD  or DROP
> 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

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Fri, May 14, 2021 at 7:58 PM vignesh C  wrote:
> I have changed it to:
>ADD adds additional publications,
> -  DROP removes publications from the list of
> +  DROP removes publications to/from the list of

How about "Publications are added to or dropped from the existing list
of publications by ADD  or DROP
respectively." ?

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

Re: alter subscription drop publication fixes

[vignesh C]

On Fri, May 14, 2021 at 7:41 AM Japin Li  wrote:
>
>
> On Thu, 13 May 2021 at 22:13, vignesh C  wrote:
> > On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
> >  wrote:
> >>
> >> On Wed, May 12, 2021 at 9:55 PM vignesh C  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.
>
> -  ADD adds additional publications,
> -  DROP removes publications from the list of
> +  ADD adds additional publications from the list
of
>
> I think, we should change the word 'from' to 'to'.

I have changed it to:
   ADD adds additional publications,
-  DROP removes publications from the list of
+  DROP removes publications to/from the list of

The changes for the same are shared in v3 patch at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm3svMg%2BhMA9GsJsUQ75HXtpjpAh2gk%3D8yZfgAnA9BMsnA%40mail.gmail.com

Regards,
Vignesh

Re: alter subscription drop publication fixes

[vignesh C]

On Thu, May 13, 2021 at 8:13 PM Bharath Rupireddy
 wrote:
>
> On Thu, May 13, 2021 at 7:43 PM vignesh C  wrote:
> > I have separated the Drop publication documentation contents. There
> > are some duplicate contents but the readability is slightly better.
> > Thoughts?
>
> -ALTER SUBSCRIPTION name
> DROP PUBLICATION  class="parameter">publication_name [, ...] [ WITH (
> set_publication_option [=
> value] [, ... ] ) ]
> +ALTER SUBSCRIPTION name
> DROP PUBLICATION  class="parameter">publication_name [, ...] [ WITH (
> refresh [= value] ) ]
>
> 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
From 3b477122f909d5e1df35c32a1775665bc92ec86b Mon Sep 17 00:00:00 2001
From: vignesh 
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
  
 
 ALTER SUBSCRIPTION name CONNECTION 'conninfo'
-ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -103,7 +103,7 @@ ALTER SUBSCRIPTION name RENAME TO <
   Changes the list of subscribed publications.  SET
   replaces the entire list of publications with a new list,
   ADD adds additional publications,
-  DROP removes publications from the list of
+  DROP removes publications to/from the list of
   publications.  See  for more
   information.  By default, this command will also act like
   REFRESH PUBLICATION, except that in case of
@@ -112,7 +112,7 @@ ALTER SUBSCRIPTION name RENAME TO <
  
 
  
-  set_publication_option specifies additional
+  publication_option specifies additional
   options for this operation.  The supported options are:
 
   
@@ -129,7 +129,8 @@ ALTER SUBSCRIPTION name RENAME TO <
   
 
   Additionally, refresh options as described
-  under REFRESH PUBLICATION may be specified.
+  under REFRESH PUBLICATION may be specified, except for
+  DROP PUBLICATION.
  
 

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 -

Re: alter subscription drop publication fixes

[Japin Li]

On Thu, 13 May 2021 at 22:13, vignesh C  wrote:
> On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
>  wrote:
>>
>> On Wed, May 12, 2021 at 9:55 PM vignesh C  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.

-  ADD adds additional publications,
-  DROP removes publications from the list of
+  ADD adds additional publications from the list of

I think, we should change the word 'from' to 'to'.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Thu, May 13, 2021 at 7:43 PM vignesh C  wrote:
> I have separated the Drop publication documentation contents. There
> are some duplicate contents but the readability is slightly better.
> Thoughts?

-ALTER SUBSCRIPTION name
DROP PUBLICATION publication_name [, ...] [ WITH (
set_publication_option [=
value] [, ... ] ) ]
+ALTER SUBSCRIPTION name
DROP PUBLICATION publication_name [, ...] [ WITH (
refresh [= value] ) ]

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

Re: alter subscription drop publication fixes

[vignesh C]

On Wed, May 12, 2021 at 10:15 PM Bharath Rupireddy
 wrote:
>
> On Wed, May 12, 2021 at 9:55 PM vignesh C  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 
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 name CONNECTION 'conninfo'
 ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ADD PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
-ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
+ALTER SUBSCRIPTION name DROP PUBLICATION publication_name [, ...] [ WITH ( refresh [= value] ) ]
 ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -94,21 +94,46 @@ ALTER SUBSCRIPTION name RENAME TO <
 

 
+   
+DROP PUBLICATION publication_name
+
+ 
+  Removes the specified publications from the list of publications.  See
+   for more information.  By
+  default the dropped publications are refreshed.
+ 
+
+ 
+  The following option is supported:
+
+  
+   
+refresh (boolean)
+
+ 
+  When false, the command will not try to refresh table information.
+  REFRESH PUBLICATION should then be executed separately.
+  The default is true.
+ 
+
+   
+  
+ 
+
+   
+

 SET PUBLICATION publication_name
 ADD PUBLICATION publication_name
-DROP PUBLICATION publication_name
 
  
   Changes the list of subscribed publications.  SET
   replaces the entire list of publications with a new list,
-  ADD adds additional publications,
-  DROP removes publications from the list of
+  ADD adds additional publications from the list of
   public

Re: alter subscription drop publication fixes

[Dilip Kumar]

On Thu, May 13, 2021 at 9:36 AM Bharath Rupireddy
 wrote:
>
> On Thu, May 13, 2021 at 8:45 AM Japin Li  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

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Thu, May 13, 2021 at 8:45 AM Japin Li  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

Re: alter subscription drop publication fixes

[Japin Li]

On Thu, 13 May 2021 at 00:45, Bharath Rupireddy 
 wrote:
> On Wed, May 12, 2021 at 9:55 PM vignesh C  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.

Re: alter subscription drop publication fixes

[Bharath Rupireddy]

On Wed, May 12, 2021 at 9:55 PM vignesh C  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

alter subscription drop publication fixes

[vignesh C]

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
From 516f7b933f06b64ebb21e3f55bdd223703879a3a Mon Sep 17 00:00:00 2001
From: vignesh 
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  ADD|DROP|SET PUBLICATION  WITH ( */
+	/* ALTER SUBSCRIPTION  ADD|SET PUBLICATION  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  DROP PUBLICATION  WITH ( */
+	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
+			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
+		COMPLETE_WITH("refresh");
 	/* ALTER SCHEMA  */
 	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