Обсуждение: 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

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: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 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