alter subscription drop publication fixes

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

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 of

I 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

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 of

The 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 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?

Regards,
Vignesh

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.

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

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/&gt;

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/&gt;

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