Documentation: warn about two_phase when altering a subscription
Hi hackers,
776621a5e4 added a "warning" in the documentation to alter a subscription (to
ensure the slot's failover property matches the subscription's one).
The same remark could be done for the two_phase option. This patch is an attempt
to do so.
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Documentation-warn-about-two_phase-when-altering-.patchtext/x-diff; charset=us-asciiDownload
From 756886e59afddd09fa6f87ab95af7292ebca3e76 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 30 Jan 2024 14:48:16 +0000
Subject: [PATCH v1] Documentation: warn about two_phase when altering a
subscription's slot name.
776621a5e4 added a warning about the newly failover option. Doing the same
for the already existing two_phase one.
---
doc/src/sgml/ref/alter_subscription.sgml | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
100.0% doc/src/sgml/ref/
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index e9e6d9d74a..cd553f6312 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -235,15 +235,17 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
When altering the
<link linkend="sql-createsubscription-params-with-slot-name"><literal>slot_name</literal></link>,
- the <literal>failover</literal> property value of the named slot may differ from the
+ the <literal>failover</literal> and <literal>two_phase</literal> properties
+ values of the named slot may differ from their counterparts
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- parameter specified in the subscription. When creating the slot,
- ensure the slot <literal>failover</literal> property matches the
+ and <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ parameters specified in the subscription. When creating the slot, ensure
+ the slot <literal>failover</literal> and <literal>two_phase</literal>
+ properties match their counterparts parameters values of the subscription.
+ Otherwise, the slot on the publisher may behave differently from what subscription's
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- parameter value of the subscription. Otherwise, the slot on the
- publisher may behave differently from what subscription's
- <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- option says. The slot on the publisher could either be
+ and <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ options say: for example, the slot on the publisher could either be
synced to the standbys even when the subscription's
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
option is disabled or could be disabled for sync
--
2.34.1
Hi, thanks for the patch. Here are some review comments for v1
======
(below is not showing the links and other sgml rendering -- all those LGTM)
BEFORE
When altering the slot_name, the failover and two_phase properties
values of the named slot may differ from their counterparts failover
and two_phase parameters specified in the subscription. When creating
the slot, ensure the slot failover and two_phase properties match
their counterparts parameters values of the subscription.
SUGGESTION
When altering the slot_name, the failover and two_phase property
values of the named slot may differ from the counterpart failover and
two_phase parameters specified by the subscription. When creating the
slot, ensure the slot properties failover and two_phase match their
counterpart parameters of the subscription.
~
BEFORE
Otherwise, the slot on the publisher may behave differently from what
subscription's failover and two_phase options say: for example, the
slot on the publisher could ...
SUGGESTION:
Otherwise, the slot on the publisher may behave differently from what
these subscription options say: for example, the slot on the publisher
could ...
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi,
On Wed, Jan 31, 2024 at 01:47:16PM +1100, Peter Smith wrote:
Hi, thanks for the patch. Here are some review comments for v1
Thanks for the review!
======
(below is not showing the links and other sgml rendering -- all those LGTM)
BEFORE
When altering the slot_name, the failover and two_phase properties
values of the named slot may differ from their counterparts failover
and two_phase parameters specified in the subscription. When creating
the slot, ensure the slot failover and two_phase properties match
their counterparts parameters values of the subscription.SUGGESTION
When altering the slot_name, the failover and two_phase property
values of the named slot may differ from the counterpart failover and
two_phase parameters specified by the subscription. When creating the
slot, ensure the slot properties failover and two_phase match their
counterpart parameters of the subscription.~
BEFORE
Otherwise, the slot on the publisher may behave differently from what
subscription's failover and two_phase options say: for example, the
slot on the publisher could ...SUGGESTION:
Otherwise, the slot on the publisher may behave differently from what
these subscription options say: for example, the slot on the publisher
could ...
As a non native English speaker somehow I have to rely on you for those
suggestions ;-)
They make sense to me so applied both in v2 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Documentation-warn-about-two_phase-when-altering-.patchtext/x-diff; charset=us-asciiDownload
From 29ef47f6a201a81557ce1b4b37b414118a623634 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 30 Jan 2024 14:48:16 +0000
Subject: [PATCH v2] Documentation: warn about two_phase when altering a
subscription's slot name.
776621a5e4 added a warning about the newly failover option. Doing the same
for the already existing two_phase one.
---
doc/src/sgml/ref/alter_subscription.sgml | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
100.0% doc/src/sgml/ref/
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index e9e6d9d74a..11f69f330d 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -235,15 +235,15 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
When altering the
<link linkend="sql-createsubscription-params-with-slot-name"><literal>slot_name</literal></link>,
- the <literal>failover</literal> property value of the named slot may differ from the
+ the <literal>failover</literal> and <literal>two_phase</literal> property
+ values of the named slot may differ from the counterpart
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- parameter specified in the subscription. When creating the slot,
- ensure the slot <literal>failover</literal> property matches the
- <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- parameter value of the subscription. Otherwise, the slot on the
- publisher may behave differently from what subscription's
- <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
- option says. The slot on the publisher could either be
+ and <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ parameters specified by the subscription. When creating the slot, ensure
+ the slot properties <literal>failover</literal> and <literal>two_phase</literal>
+ match their counterpart parameters of the subscription.
+ Otherwise, the slot on the publisher may behave differently from what these
+ subscription options say: for example, the slot on the publisher could either be
synced to the standbys even when the subscription's
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
option is disabled or could be disabled for sync
--
2.34.1
On Wed, Jan 31, 2024 at 4:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
...
As a non native English speaker somehow I have to rely on you for those
suggestions ;-)They make sense to me so applied both in v2 attached.
Patch v2 looks OK to me, but probably there is still room for
improvement. Let's see what others think.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 31, 2024 at 11:25 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
They make sense to me so applied both in v2 attached.
This patch looks good to me but I think it is better to commit this
after we push some more work on failover slots, especially the core
sync slot functionality because we are changing the existing wording
of the related work.
--
With Regards,
Amit Kapila.
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed
Hello,
I've reviewed your patch, it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing
parameters specified by the subscription. When creating the slot, ensure
to
parameters specified in the subscription. When creating the slot, ensure
and secondly the section "ensure the slot properties failover and two_phase match their counterpart parameters of the subscription" sounds a bit clunky. So I'd also propose changing:
the slot properties <literal>failover</literal> and <literal>two_phase</literal>
match their counterpart parameters of the subscription.
to
the slot properties <literal>failover</literal> and <literal>two_phase</literal>
match their counterpart parameters in the subscription.
I feel this makes the description flow a bit better when reading. But other than that I think it's quite clear.
kind regards,
-----------------------
Tristen Raab
Highgo Software Canada
www.highgo.ca
Hi,
On Fri, Feb 23, 2024 at 11:50:17PM +0000, Tristen Raab wrote:
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passedHello,
I've reviewed your patch,
Thanks!
it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing
parameters specified by the subscription. When creating the slot, ensure
to
parameters specified in the subscription. When creating the slot, ensure
As non native English speaker I don't have a strong opinion on that (though I
also preferred how it was worded in V1).
and secondly the section "ensure the slot properties failover and two_phase match their counterpart parameters of the subscription" sounds a bit clunky. So I'd also propose changing:
the slot properties <literal>failover</literal> and <literal>two_phase</literal>
match their counterpart parameters of the subscription.to
the slot properties <literal>failover</literal> and <literal>two_phase</literal>
match their counterpart parameters in the subscription.
Same here, I don't have a strong opinion on that.
As the patch as it is now looks good to Amit (see [1]/messages/by-id/CAA4eK1KdZMtJfo=K=XWsQQu8OHutT_dwJV2D3zs4h9g5zdNz2A@mail.gmail.com), I prefer to let him
decide which wording he pefers to push.
I feel this makes the description flow a bit better when reading. But other than that I think it's quite clear.
Thanks!
[1]: /messages/by-id/CAA4eK1KdZMtJfo=K=XWsQQu8OHutT_dwJV2D3zs4h9g5zdNz2A@mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On 26 Feb 2024, at 14:14, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
As the patch as it is now looks good to Amit (see [1]), I prefer to let him
decide which wording he pefers to push.
Added Amit to cc, just to be sure. Thanks!
Best regards, Andrey Borodin, learning how to be CFM.
On Sat, Mar 2, 2024 at 11:44 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
On 26 Feb 2024, at 14:14, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
As the patch as it is now looks good to Amit (see [1]), I prefer to let him
decide which wording he pefers to push.Added Amit to cc, just to be sure. Thanks!
I'll look into it during this CF.
--
With Regards,
Amit Kapila.
On Sat, Feb 24, 2024 at 5:21 AM Tristen Raab <tristen.raab@highgo.ca> wrote:
I've reviewed your patch, it applies correctly and the documentation builds without any errors. As for the content of the patch itself, I think so far it's good but would make two modifications. I like how the patch was worded originally when referring to the subscription, stating these parameters were 'in' the subscription rather than 'by' it. So I'd propose changing
parameters specified by the subscription. When creating the slot, ensure
to
parameters specified in the subscription. When creating the slot, ensure
This suggestion makes sense, so I updated the patch for this and committed.
--
With Regards,
Amit Kapila.