Обсуждение: [DOC] Update ALTER SUBSCRIPTION documentation

Hi,

We have recently used the PostgreSQL documentation when setting up our
logical replication. We noticed there was a step missing in the
documentation on how to drop a logical replication subscription with a
replication slot attached.

We clarify the documentation to include prerequisites for running the
DROP SUBSCRIPTION command. Please see attached patch.

Best regards,
Robert Sjöblom,
Oscar Carlberg
--
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag
gått till fel adressat vänligen radera det ursprungliga meddelandet och
underrätta avsändaren via e-post

On Fri, May 5, 2023 at 6:47 PM Robert Sjöblom <robert.sjoblom@fortnox.se> wrote:
>
> We have recently used the PostgreSQL documentation when setting up our
> logical replication. We noticed there was a step missing in the
> documentation on how to drop a logical replication subscription with a
> replication slot attached.
>
> We clarify the documentation to include prerequisites for running the
> DROP SUBSCRIPTION command. Please see attached patch.
>

Shouldn't we also change the following errhint in the code as well?
ReportSlotConnectionError()
{
...
ereport(ERROR,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not connect to publisher when attempting to drop
replication slot \"%s\": %s",
slotname, err),
/* translator: %s is an SQL ALTER command */
errhint("Use %s to disassociate the subscription from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
...
}

--
With Regards,
Amit Kapila.

On Fri, May 5, 2023 at 11:17 PM Robert Sjöblom
<robert.sjoblom@fortnox.se> wrote:
>
>
> Hi,
>
> We have recently used the PostgreSQL documentation when setting up our
> logical replication. We noticed there was a step missing in the
> documentation on how to drop a logical replication subscription with a
> replication slot attached.
>
> We clarify the documentation to include prerequisites for running the
> DROP SUBSCRIPTION command. Please see attached patch.

Right, there is a "missing step" in the documentation, but OTOH that
step is going to be obvious from the error you get when attempting to
set the slot_name to NONE:

e.g.
test_sub=# ALTER SUBSCRIPTION sub1 SET (slot_name= NONE);
ERROR:  cannot set slot_name = NONE for enabled subscription

~

IMO this scenario is sort of a trade-off between (a) wanting to give
every little step explicitly versus (b) trying to keep the
documentation free of clutter.

I think a comprise here is just to mention the need for disabling the
subscription but without spelling out the details of the ALTER ...
DISABLE command.

For example,

BEFORE
To proceed in this situation, disassociate the subscription from the
replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name =
NONE).

SUGGESTION
To proceed in this situation, first DISABLE the subscription, and then
disassociate it from the replication slot by executing ALTER
SUBSCRIPTION ... SET (slot_name = NONE).

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, May 8, 2023 at 12:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 5, 2023 at 6:47 PM Robert Sjöblom <robert.sjoblom@fortnox.se> wrote:
> >
> > We have recently used the PostgreSQL documentation when setting up our
> > logical replication. We noticed there was a step missing in the
> > documentation on how to drop a logical replication subscription with a
> > replication slot attached.
> >
> > We clarify the documentation to include prerequisites for running the
> > DROP SUBSCRIPTION command. Please see attached patch.
> >
>
> Shouldn't we also change the following errhint in the code as well?
> ReportSlotConnectionError()
> {
> ...
> ereport(ERROR,
> (errcode(ERRCODE_CONNECTION_FAILURE),
> errmsg("could not connect to publisher when attempting to drop
> replication slot \"%s\": %s",
> slotname, err),
> /* translator: %s is an SQL ALTER command */
> errhint("Use %s to disassociate the subscription from the slot.",
> "ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
> ...
> }

Yeah, if the subscription is enabled, it might be helpful for users if
the error hint message says something like:

Use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate the
subscription from the slot after disabling the subscription.

Apart from the documentation change, given that setting slot_name =
NONE always requires for the subscription to be disabled beforehand,
does it make sense to change ALTER SUBSCRIPTION SET so that we disable
the subscription when setting slot_name = NONE? Setting slot_name to a
valid slot name doesn't enable the subscription, though.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, May 8, 2023 at 1:51 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Apart from the documentation change, given that setting slot_name =
> NONE always requires for the subscription to be disabled beforehand,
> does it make sense to change ALTER SUBSCRIPTION SET so that we disable
> the subscription when setting slot_name = NONE? Setting slot_name to a
> valid slot name doesn't enable the subscription, though.
>

I think this is worth considering. Offhand, I don't see any problem
with this idea but users may not like the automatic disabling of
subscriptions along with setting slot_name=NONE. It would be better to
discuss this in a separate thread to seek the opinion of others.

--
With Regards,
Amit Kapila.

On Tue, May 9, 2023 at 3:40 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, May 8, 2023 at 1:51 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Apart from the documentation change, given that setting slot_name =
> > NONE always requires for the subscription to be disabled beforehand,
> > does it make sense to change ALTER SUBSCRIPTION SET so that we disable
> > the subscription when setting slot_name = NONE? Setting slot_name to a
> > valid slot name doesn't enable the subscription, though.
> >
>
> I think this is worth considering. Offhand, I don't see any problem
> with this idea but users may not like the automatic disabling of
> subscriptions along with setting slot_name=NONE. It would be better to
> discuss this in a separate thread to seek the opinion of others.
>

Agreed.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On 2023-05-05 15:17, Robert Sjöblom wrote:
>
> Hi,
>
> We have recently used the PostgreSQL documentation when setting up our
> logical replication. We noticed there was a step missing in the
> documentation on how to drop a logical replication subscription with a
> replication slot attached.

Following discussions, please see revised documentation patch.

Best regards,
Robert Sjöblom
--
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag
gått till fel adressat vänligen radera det ursprungliga meddelandet och
underrätta avsändaren via e-post

On Mon, May 15, 2023 at 11:36 PM Robert Sjöblom
<robert.sjoblom@fortnox.se> wrote:
>
>
>
> On 2023-05-05 15:17, Robert Sjöblom wrote:
> >
> > Hi,
> >
> > We have recently used the PostgreSQL documentation when setting up our
> > logical replication. We noticed there was a step missing in the
> > documentation on how to drop a logical replication subscription with a
> > replication slot attached.
>
> Following discussions, please see revised documentation patch.
>

LGTM.

BTW, in the previous thread, there was also a suggestion from Amit [1]
to change the errhint in a similar way. There was no reply to Amit's
idea, so it's not clear whether it's an accidental omission from your
v2 patch or not.

------
[1] https://www.postgresql.org/message-id/CAA4eK1J11phiaoCOmsjNqPZ9BOWyLXYrfgrm5vU2uCFPF2kN1Q%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Den tis 16 maj 2023 kl 01:44 skrev Peter Smith <smithpb2250@gmail.com>:
>
> On Mon, May 15, 2023 at 11:36 PM Robert Sjöblom
> <robert.sjoblom@fortnox.se> wrote:
> >
> >
> >
> > On 2023-05-05 15:17, Robert Sjöblom wrote:
> > >
> > > Hi,
> > >
> > > We have recently used the PostgreSQL documentation when setting up our
> > > logical replication. We noticed there was a step missing in the
> > > documentation on how to drop a logical replication subscription with a
> > > replication slot attached.
> >
> > Following discussions, please see revised documentation patch.
> >
>
> LGTM.
>
> BTW, in the previous thread, there was also a suggestion from Amit [1]
> to change the errhint in a similar way. There was no reply to Amit's
> idea, so it's not clear whether it's an accidental omission from your
> v2 patch or not.
>
> ------
> [1] https://www.postgresql.org/message-id/CAA4eK1J11phiaoCOmsjNqPZ9BOWyLXYrfgrm5vU2uCFPF2kN1Q%40mail.gmail.com
>
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

Accidental omission by way of mail client, I suppose -- some messages
got flagged as spam and moved to another folder. I went ahead with
Masahiko Sawada's suggestion for the error message; see revised patch.

Best regards,
Robert Sjöblom

--
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag
gått till fel adressat vänligen radera det ursprungliga meddelandet och
underrätta avsändaren via e-post

+ errhint("Use %s to disassociate the subscription from the slot after
disabling the subscription.",

IMO it looked strange having the word "subscription" 2x in the same sentence.

Maybe you can reword the errhint like:

BEFORE
"Use %s to disassociate the subscription from the slot after disabling
the subscription."

SUGGESTION#1
"Disable the subscription, then use %s to disassociate it from the slot."

SUGGESTION#2
"After disabling the subscription use %s to disassociate it from the slot."

~~~

BTW, it is a bit difficult to follow this thread because the subject
keeps changing.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Den ons 17 maj 2023 kl 03:18 skrev Peter Smith <smithpb2250@gmail.com>:
>
> + errhint("Use %s to disassociate the subscription from the slot after
> disabling the subscription.",
>
> IMO it looked strange having the word "subscription" 2x in the same sentence.
>
> Maybe you can reword the errhint like:
>
> BEFORE
> "Use %s to disassociate the subscription from the slot after disabling
> the subscription."
>
> SUGGESTION#1
> "Disable the subscription, then use %s to disassociate it from the slot."
>
> SUGGESTION#2
> "After disabling the subscription use %s to disassociate it from the slot."
>
> ~~~
>
> BTW, it is a bit difficult to follow this thread because the subject
> keeps changing.
>
> ------
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

Good catch, I definitely agree. I'm sorry about changing the subject
line, I'm unaccustomed to mailing lists -- I'll leave it as it is now.

Attached is the revised version.

Best regards,
Robert Sjöblom

--
Innehållet i detta e-postmeddelande är konfidentiellt och avsett endast för
adressaten.Varje spridning, kopiering eller utnyttjande av innehållet är
förbjuden utan tillåtelse av avsändaren. Om detta meddelande av misstag
gått till fel adressat vänligen radera det ursprungliga meddelandet och
underrätta avsändaren via e-post

On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom
<robert.sjoblom@fortnox.se> wrote:
>
> Den ons 17 maj 2023 kl 03:18 skrev Peter Smith <smithpb2250@gmail.com>:
> >
> > + errhint("Use %s to disassociate the subscription from the slot after
> > disabling the subscription.",
> >
> > IMO it looked strange having the word "subscription" 2x in the same sentence.
> >
> > Maybe you can reword the errhint like:
> >
> > BEFORE
> > "Use %s to disassociate the subscription from the slot after disabling
> > the subscription."
> >
> > SUGGESTION#1
> > "Disable the subscription, then use %s to disassociate it from the slot."
> >
> > SUGGESTION#2
> > "After disabling the subscription use %s to disassociate it from the slot."
> >
> > ~~~
> >
> > BTW, it is a bit difficult to follow this thread because the subject
> > keeps changing.
> >
> > ------
> > Kind Regards,
> > Peter Smith.
> > Fujitsu Australia
>
> Good catch, I definitely agree. I'm sorry about changing the subject
> line, I'm unaccustomed to mailing lists -- I'll leave it as it is now.
>
> Attached is the revised version.
>

v4 looks good to me.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, May 17, 2023 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom
> <robert.sjoblom@fortnox.se> wrote:
> >
> > Attached is the revised version.
> >
>
> v4 looks good to me.
>

The latest version looks good to me as well. I think we should
backpatch this change as this is a user-facing message change in docs
and code. What do you guys think?

--
With Regards,
Amit Kapila.

On Wed, Jun 14, 2023 at 1:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 17, 2023 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom
> > <robert.sjoblom@fortnox.se> wrote:
> > >
> > > Attached is the revised version.
> > >
> >
> > v4 looks good to me.
> >
>
> The latest version looks good to me as well. I think we should
> backpatch this change as this is a user-facing message change in docs
> and code. What do you guys think?
>

I do not know the exact criteria for deciding to back-patch, but I am
not sure back-patching is so important for this one.

It is not a critical bug-fix, and despite being a user-facing change,
there is no functional change. Also, IIUC the previous docs existed
for 6 years without problem.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Jun 14, 2023 at 9:25 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jun 14, 2023 at 1:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, May 17, 2023 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom
> > > <robert.sjoblom@fortnox.se> wrote:
> > > >
> > > > Attached is the revised version.
> > > >
> > >
> > > v4 looks good to me.
> > >
> >
> > The latest version looks good to me as well. I think we should
> > backpatch this change as this is a user-facing message change in docs
> > and code. What do you guys think?
> >
>
> I do not know the exact criteria for deciding to back-patch, but I am
> not sure back-patching is so important for this one.
>
> It is not a critical bug-fix, and despite being a user-facing change,
> there is no functional change.
>

Right neither this is a functional change nor a critical but where any
work will be stopped due to this but I think we do prefer to backpatch
changes (doc) where user-facing docs have an additional explanation.
For example, see [1][2]. OTOH, there is an argument that we should do
this only in v17 but I guess this is a simple improvement that will be
helpful for even current users, so it is better to change this in
existing branches as well.

[1] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e126d817c7af989c47366b0e344ee83d761f334a
[2] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f170b572d2b4cc232c5b6d391b4ecf3e368594b7

--
With Regards,
Amit Kapila.

On 14.06.23 05:09, Amit Kapila wrote:
> The latest version looks good to me as well. I think we should
> backpatch this change as this is a user-facing message change in docs
> and code. What do you guys think?

Isn't that a reason *not* to backpatch it?

On Wed, Jun 14, 2023 at 6:52 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 14.06.23 05:09, Amit Kapila wrote:
> > The latest version looks good to me as well. I think we should
> > backpatch this change as this is a user-facing message change in docs
> > and code. What do you guys think?
>
> Isn't that a reason *not* to backpatch it?
>

I wanted to backpatch the following change which provides somewhat
accurate information about what a user needs to do when it faces an
error.
To proceed in
-   this situation, disassociate the subscription from the replication slot by
-   executing <literal>ALTER SUBSCRIPTION ... SET (slot_name = NONE)</literal>.
+   this situation, first <literal>DISABLE</literal> the subscription, and then
+   disassociate it from the replication slot by executing
+   <literal>ALTER SUBSCRIPTION ... SET (slot_name = NONE)</literal>.

Now, along with this change, there is a change in errhint as well
which I am not sure about whether to backpatch or not. I think we have
the following options (a) commit both doc and code change in HEAD (b)
commit both doc and code change in v17 when the next version branch
opens (c) backpatch the doc change and commit the code change in HEAD
only (d) backpatch the doc change and commit the code change in v17
(e) backpatch both the doc and code change.

What do you think?

--
With Regards,
Amit Kapila.

On 15.06.23 04:49, Amit Kapila wrote:
> I wanted to backpatch the following change which provides somewhat
> accurate information about what a user needs to do when it faces an
> error.
> To proceed in
> -   this situation, disassociate the subscription from the replication slot by
> -   executing <literal>ALTER SUBSCRIPTION ... SET (slot_name = NONE)</literal>.
> +   this situation, first <literal>DISABLE</literal> the subscription, and then
> +   disassociate it from the replication slot by executing
> +   <literal>ALTER SUBSCRIPTION ... SET (slot_name = NONE)</literal>.
> 
> Now, along with this change, there is a change in errhint as well
> which I am not sure about whether to backpatch or not. I think we have
> the following options (a) commit both doc and code change in HEAD (b)
> commit both doc and code change in v17 when the next version branch
> opens (c) backpatch the doc change and commit the code change in HEAD
> only (d) backpatch the doc change and commit the code change in v17
> (e) backpatch both the doc and code change.

Reading the thread again now, I think this is essentially a bug fix, so 
I don't mind backpatching it.

I wish the errhint would show the actual command to disable the 
subscription.  It already shows the command to detach the replication 
slot, so it would only be consistent to also show the other command.

On Fri, Jun 16, 2023 at 7:15 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 15.06.23 04:49, Amit Kapila wrote:
> >
> > Now, along with this change, there is a change in errhint as well
> > which I am not sure about whether to backpatch or not. I think we have
> > the following options (a) commit both doc and code change in HEAD (b)
> > commit both doc and code change in v17 when the next version branch
> > opens (c) backpatch the doc change and commit the code change in HEAD
> > only (d) backpatch the doc change and commit the code change in v17
> > (e) backpatch both the doc and code change.
>
> Reading the thread again now, I think this is essentially a bug fix, so
> I don't mind backpatching it.
>
> I wish the errhint would show the actual command to disable the
> subscription.  It already shows the command to detach the replication
> slot, so it would only be consistent to also show the other command.
>

Fair enough. I updated the errhint and slightly adjusted the docs as
well in the attached.

--
With Regards,
Amit Kapila.

FYI - I have created and tested back-patches for Amit's v5 patch,
going all the way to REL_10_STABLE.

(the patches needed tweaking several times due to minor code/docs
differences in the earlier versions)

PSA.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Jun 20, 2023 at 9:02 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> FYI - I have created and tested back-patches for Amit's v5 patch,
> going all the way to REL_10_STABLE.
>

Pushed. I haven't used PG10 patch as REL_10_STABLE is out of support now.

--
With Regards,
Amit Kapila.