Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Tue, Jun 20, 2023 at 9:02 AM Peter Smith 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.
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
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 logical_replication_slot_disable_doc_update_v5_REL_12.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_REL_11.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_HEAD.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_REL_13.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_REL_10.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_REL_15.patch Description: Binary data logical_replication_slot_disable_doc_update_v5_REL_14.patch Description: Binary data
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Fri, Jun 16, 2023 at 7:15 PM Peter Eisentraut 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. logical_replication_slot_disable_doc_update_v5.patch Description: Binary data
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
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 ALTER SUBSCRIPTION ... SET (slot_name = NONE). + this situation, first DISABLE the subscription, and then + disassociate it from the replication slot by executing + ALTER SUBSCRIPTION ... SET (slot_name = NONE). 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.
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Wed, Jun 14, 2023 at 6:52 PM Peter Eisentraut 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 ALTER SUBSCRIPTION ... SET (slot_name = NONE). + this situation, first DISABLE the subscription, and then + disassociate it from the replication slot by executing + ALTER SUBSCRIPTION ... SET (slot_name = NONE). 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.
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
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?
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Wed, Jun 14, 2023 at 9:25 AM Peter Smith wrote: > > On Wed, Jun 14, 2023 at 1:10 PM Amit Kapila wrote: > > > > On Wed, May 17, 2023 at 11:57 AM Peter Smith wrote: > > > > > > On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom > > > 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.
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Wed, Jun 14, 2023 at 1:10 PM Amit Kapila wrote: > > On Wed, May 17, 2023 at 11:57 AM Peter Smith wrote: > > > > On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom > > 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
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Wed, May 17, 2023 at 11:57 AM Peter Smith wrote: > > On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom > 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.
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
On Wed, May 17, 2023 at 2:53 PM Robert Sjöblom
wrote:
>
> Den ons 17 maj 2023 kl 03:18 skrev Peter Smith :
> >
> > + 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
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
Den ons 17 maj 2023 kl 03:18 skrev Peter Smith :
>
> + 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
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 8d997c983f..4be6ddb873 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -86,8 +86,9 @@ DROP SUBSCRIPTION [ IF EXISTS ] nameDROP SUBSCRIPTION command will fail. To proceed in
- this situation, disassociate the subscription from the replication slot by
- executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+ this situation, first DISABLE the subscription, and then
+ disassociate it from the replication slot by executing
+ ALTER SUBSCRIPTION ... SET (slot_name = NONE).
After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host. Note that if the remote replication slot still
exists, it (and any related table synchronization slots) should then be
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e8b288d01c..c0373e5fad 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2185,7 +2185,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
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.",
+ errhint("Disable the subscription, then use %s to disassociate it from the slot.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
}
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
+ 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
Re: [DOC] Update ALTER SUBSCRIPTION documentation v3
Den tis 16 maj 2023 kl 01:44 skrev Peter Smith :
>
> On Mon, May 15, 2023 at 11:36 PM Robert Sjöblom
> 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
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 8d997c983f..4be6ddb873 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -86,8 +86,9 @@ DROP SUBSCRIPTION [ IF EXISTS ] nameDROP SUBSCRIPTION command will fail. To proceed in
- this situation, disassociate the subscription from the replication slot by
- executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+ this situation, first DISABLE the subscription, and then
+ disassociate it from the replication slot by executing
+ ALTER SUBSCRIPTION ... SET (slot_name = NONE).
After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host. Note that if the remote replication slot still
exists, it (and any related table synchronization slots) should then be
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e8b288d01c..9ecb91ab15 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2185,7 +2185,7 @@ ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err)
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.",
+ errhint("Use %s to disassociate the subscription from the slot after disabling the subscription.",
"ALTER SUBSCRIPTION ... SET (slot_name = NONE)")));
}
