Обсуждение: Disallow changing slot's failover option in transaction block

Hi,

Kuroda-San reported an issue off-list that:

If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.

This is because ALTER SUBSCRIPTION SET (failover) command internally executes
the replication command ALTER_REPLICATION_SLOT to change the replication slot's
failover property, but this replication command execution cannot be
rollback.

To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.

Best Regards,
Hou Zhijie

Hi,

On Tue, Apr 16, 2024 at 06:32:11AM +0000, Zhijie Hou (Fujitsu) wrote:
> Hi,
> 
> Kuroda-San reported an issue off-list that:
> 
> If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
> and rollback, only the subscription option change can be rolled back, while the
> replication slot's failover change is preserved.

Nice catch, thanks!

> To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
> (failover) inside a txn block, which is also consistent to the ALTER
> SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
> patch to address this.

Agree. The patch looks pretty straightforward to me. Worth to add this
case in the doc? (where we already mention that "Commands ALTER SUBSCRIPTION ...
REFRESH PUBLICATION and ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ...
with refresh option as true cannot be executed inside a transaction block"

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Dear Hou,

> Kuroda-San reported an issue off-list that:
>
> If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
> and rollback, only the subscription option change can be rolled back, while the
> replication slot's failover change is preserved.
>
> This is because ALTER SUBSCRIPTION SET (failover) command internally
> executes
> the replication command ALTER_REPLICATION_SLOT to change the replication
> slot's
> failover property, but this replication command execution cannot be
> rollback.
>
> To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
> (failover) inside a txn block, which is also consistent to the ALTER
> SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
> patch to address this.

Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1] can be updated.


[1]:https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Hou,
>
> > Kuroda-San reported an issue off-list that:
> >
> > If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
> > and rollback, only the subscription option change can be rolled back, while the
> > replication slot's failover change is preserved.
> >
> > This is because ALTER SUBSCRIPTION SET (failover) command internally
> > executes
> > the replication command ALTER_REPLICATION_SLOT to change the replication
> > slot's
> > failover property, but this replication command execution cannot be
> > rollback.
> >
> > To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
> > (failover) inside a txn block, which is also consistent to the ALTER
> > SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
> > patch to address this.
>
> Thanks for posting the patch, the fix is same as my expectation.
> Also, should we add the restriction to the doc? I feel [1] can be updated.

+1.

Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).

So, we need to think if there are better ways to fix it.  After
discussion with Hou-San offlist, here are some ideas:

1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION   WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.

This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.

2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.

This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.

3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
the  slot's failover if alter_slot=true. And so we can disallow CREATE
SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.

This seems a clean way, as everything will be as per user's consent
based on alter_slot parameter. But on the downside, this will need
introducing additional parameter and also adding new restriction of
running CREATE-sub in txn  block for a specific case.

4. Don't alter replication in subscription DDLs. Instead, try to alter
replication slot's failover in the apply worker. This means we need to
execute alter_replication_slot each time before starting streaming in
the apply worker.

This does not seem appealing to execute alter_replication_slot
everytime the apply worker starts. But if others think it as a better
option, it can be further analyzed.


Currently, our preference is option 2, as that looks a clean solution
and also aligns with ALTER-SUB behavior which is already documented.
Thoughts?

--------------------------------
Note that we could not refer to the design of two_phase here, because
two_phase can be considered as a streaming option, so it's fine to
change the two_phase along with START_REPLICATION command. (the
two_phase is not changed in subscription DDLs, but get changed in
START_REPLICATION command). But the failover is closely related to a
replication slot itself.
--------------------------------


Thanks
Shveta

On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Hou,
> >
> > > Kuroda-San reported an issue off-list that:
> > >
> > > If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
> > > and rollback, only the subscription option change can be rolled back, while the
> > > replication slot's failover change is preserved.
> > >
> > > This is because ALTER SUBSCRIPTION SET (failover) command internally
> > > executes
> > > the replication command ALTER_REPLICATION_SLOT to change the replication
> > > slot's
> > > failover property, but this replication command execution cannot be
> > > rollback.
> > >
> > > To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
> > > (failover) inside a txn block, which is also consistent to the ALTER
> > > SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
> > > patch to address this.
> >
> > Thanks for posting the patch, the fix is same as my expectation.
> > Also, should we add the restriction to the doc? I feel [1] can be updated.
>
> +1.
>
> Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
> because we call alter_replication_slot in CREATE SUB as well, for the
> case when slot_name is provided and create_slot=false. But the tricky
> part is we call alter_replication_slot() when creating subscription
> for both failover=true and false. That means if we want to fix it on
> the similar line of ALTER SUB, we have to disallow user from executing
> the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
> to break some existing use cases. (previously, user can execute such a
> command inside a txn block).
>
> So, we need to think if there are better ways to fix it.  After
> discussion with Hou-San offlist, here are some ideas:
>
> 1. do not alter replication slot's failover option when CREATE
> SUBSCRIPTION   WITH failover=false. This means we alter replication
> slot only when failover is set to true. And thus we can disallow
> CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
> inside a txn block.
>
> This option allows user to run CREATE-SUB(create_slot=false) with
> failover=false in txn block like earlier. But on the downside, it
> makes the behavior inconsistent for otherwise simpler option like
> failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
> block while with failover=false, it is allowed. It makes it slightly
> complex to be understood by user.
>
> 2. let's not disallow CREATE SUB in txn block as earlier, just don't
> perform internal alter-failover during CREATE SUB for existing
> slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
> false, we will ignore failover parameter of CREATE SUB and it is
> user's responsibility to set it appropriately using ALTER SUB cmd. For
> create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
>
> This option does not add new restriction for CREATE SUB wrt txn block.
> In context of failover with create_slot=false, we already have a
> similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
> SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
> failover by himself. CREAT SUB can also be documented in similar way.
> This seems simpler to be understood considering existing ALTER SUB's
> behavior as well. Plus, this will make CREATE-SUB code slightly
> simpler and thus easily manageable.
>

+1 for option 2 as it sounds logical to me and consistent with ALTER
SUBSCRIPTION. BTW, IIUC, you are referring to: "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 in the subscription. When creating the slot, ensure the slot
properties failover and two_phase match their counterpart parameters
of the subscription." in docs [1], right?

--
With Regards,
Amit Kapila.

On Thu, Apr 18, 2024 at 11:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Hou,
> > >
> > > > Kuroda-San reported an issue off-list that:
> > > >
> > > > If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
> > > > and rollback, only the subscription option change can be rolled back, while the
> > > > replication slot's failover change is preserved.
> > > >
> > > > This is because ALTER SUBSCRIPTION SET (failover) command internally
> > > > executes
> > > > the replication command ALTER_REPLICATION_SLOT to change the replication
> > > > slot's
> > > > failover property, but this replication command execution cannot be
> > > > rollback.
> > > >
> > > > To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
> > > > (failover) inside a txn block, which is also consistent to the ALTER
> > > > SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
> > > > patch to address this.
> > >
> > > Thanks for posting the patch, the fix is same as my expectation.
> > > Also, should we add the restriction to the doc? I feel [1] can be updated.
> >
> > +1.
> >
> > Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
> > because we call alter_replication_slot in CREATE SUB as well, for the
> > case when slot_name is provided and create_slot=false. But the tricky
> > part is we call alter_replication_slot() when creating subscription
> > for both failover=true and false. That means if we want to fix it on
> > the similar line of ALTER SUB, we have to disallow user from executing
> > the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
> > to break some existing use cases. (previously, user can execute such a
> > command inside a txn block).
> >
> > So, we need to think if there are better ways to fix it.  After
> > discussion with Hou-San offlist, here are some ideas:
> >
> > 1. do not alter replication slot's failover option when CREATE
> > SUBSCRIPTION   WITH failover=false. This means we alter replication
> > slot only when failover is set to true. And thus we can disallow
> > CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
> > inside a txn block.
> >
> > This option allows user to run CREATE-SUB(create_slot=false) with
> > failover=false in txn block like earlier. But on the downside, it
> > makes the behavior inconsistent for otherwise simpler option like
> > failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
> > block while with failover=false, it is allowed. It makes it slightly
> > complex to be understood by user.
> >
> > 2. let's not disallow CREATE SUB in txn block as earlier, just don't
> > perform internal alter-failover during CREATE SUB for existing
> > slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
> > false, we will ignore failover parameter of CREATE SUB and it is
> > user's responsibility to set it appropriately using ALTER SUB cmd. For
> > create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
> >
> > This option does not add new restriction for CREATE SUB wrt txn block.
> > In context of failover with create_slot=false, we already have a
> > similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
> > SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
> > failover by himself. CREAT SUB can also be documented in similar way.
> > This seems simpler to be understood considering existing ALTER SUB's
> > behavior as well. Plus, this will make CREATE-SUB code slightly
> > simpler and thus easily manageable.
> >
>
> +1 for option 2 as it sounds logical to me and consistent with ALTER
> SUBSCRIPTION. BTW, IIUC, you are referring to: "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 in the subscription. When creating the slot, ensure the slot
> properties failover and two_phase match their counterpart parameters
> of the subscription." in docs [1], right?

Yes. Here:
https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET

thanks
Shveta

Dear Shveta,

Sorry for delay response. I missed your post.

> +1.
> 
> Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
> because we call alter_replication_slot in CREATE SUB as well, for the
> case when slot_name is provided and create_slot=false. But the tricky
> part is we call alter_replication_slot() when creating subscription
> for both failover=true and false. That means if we want to fix it on
> the similar line of ALTER SUB, we have to disallow user from executing
> the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
> to break some existing use cases. (previously, user can execute such a
> command inside a txn block).
> 
> So, we need to think if there are better ways to fix it.  After
> discussion with Hou-San offlist, here are some ideas:
> 1. do not alter replication slot's failover option when CREATE
> SUBSCRIPTION   WITH failover=false. This means we alter replication
> slot only when failover is set to true. And thus we can disallow
> CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
> inside a txn block.
> 
> This option allows user to run CREATE-SUB(create_slot=false) with
> failover=false in txn block like earlier. But on the downside, it
> makes the behavior inconsistent for otherwise simpler option like
> failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
> block while with failover=false, it is allowed. It makes it slightly
> complex to be understood by user.
> 
> 2. let's not disallow CREATE SUB in txn block as earlier, just don't
> perform internal alter-failover during CREATE SUB for existing
> slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
> false, we will ignore failover parameter of CREATE SUB and it is
> user's responsibility to set it appropriately using ALTER SUB cmd. For
> create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
> 
> This option does not add new restriction for CREATE SUB wrt txn block.
> In context of failover with create_slot=false, we already have a
> similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
> SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
> failover by himself. CREAT SUB can also be documented in similar way.
> This seems simpler to be understood considering existing ALTER SUB's
> behavior as well. Plus, this will make CREATE-SUB code slightly
> simpler and thus easily manageable.
> 
> 3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
> the  slot's failover if alter_slot=true. And so we can disallow CREATE
> SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.
> 
> This seems a clean way, as everything will be as per user's consent
> based on alter_slot parameter. But on the downside, this will need
> introducing additional parameter and also adding new restriction of
> running CREATE-sub in txn  block for a specific case.
> 
> 4. Don't alter replication in subscription DDLs. Instead, try to alter
> replication slot's failover in the apply worker. This means we need to
> execute alter_replication_slot each time before starting streaming in
> the apply worker.
> 
> This does not seem appealing to execute alter_replication_slot
> everytime the apply worker starts. But if others think it as a better
> option, it can be further analyzed.

Thanks for describing, I also prefer 2, because it seems bit strange that
CREATE statement leads ALTER.

> Currently, our preference is option 2, as that looks a clean solution
> and also aligns with ALTER-SUB behavior which is already documented.
> Thoughts?
> 
> --------------------------------
> Note that we could not refer to the design of two_phase here, because
> two_phase can be considered as a streaming option, so it's fine to
> change the two_phase along with START_REPLICATION command. (the
> two_phase is not changed in subscription DDLs, but get changed in
> START_REPLICATION command). But the failover is closely related to a
> replication slot itself.
> --------------------------------

Sorry, I cannot find statements. Where did you refer?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

On Thu, Apr 18, 2024 at 11:40 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shveta,
>
> Sorry for delay response. I missed your post.
>
> > +1.
> >
> > Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
> > because we call alter_replication_slot in CREATE SUB as well, for the
> > case when slot_name is provided and create_slot=false. But the tricky
> > part is we call alter_replication_slot() when creating subscription
> > for both failover=true and false. That means if we want to fix it on
> > the similar line of ALTER SUB, we have to disallow user from executing
> > the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
> > to break some existing use cases. (previously, user can execute such a
> > command inside a txn block).
> >
> > So, we need to think if there are better ways to fix it.  After
> > discussion with Hou-San offlist, here are some ideas:
> > 1. do not alter replication slot's failover option when CREATE
> > SUBSCRIPTION   WITH failover=false. This means we alter replication
> > slot only when failover is set to true. And thus we can disallow
> > CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
> > inside a txn block.
> >
> > This option allows user to run CREATE-SUB(create_slot=false) with
> > failover=false in txn block like earlier. But on the downside, it
> > makes the behavior inconsistent for otherwise simpler option like
> > failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
> > block while with failover=false, it is allowed. It makes it slightly
> > complex to be understood by user.
> >
> > 2. let's not disallow CREATE SUB in txn block as earlier, just don't
> > perform internal alter-failover during CREATE SUB for existing
> > slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
> > false, we will ignore failover parameter of CREATE SUB and it is
> > user's responsibility to set it appropriately using ALTER SUB cmd. For
> > create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
> >
> > This option does not add new restriction for CREATE SUB wrt txn block.
> > In context of failover with create_slot=false, we already have a
> > similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
> > SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
> > failover by himself. CREAT SUB can also be documented in similar way.
> > This seems simpler to be understood considering existing ALTER SUB's
> > behavior as well. Plus, this will make CREATE-SUB code slightly
> > simpler and thus easily manageable.
> >
> > 3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
> > the  slot's failover if alter_slot=true. And so we can disallow CREATE
> > SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.
> >
> > This seems a clean way, as everything will be as per user's consent
> > based on alter_slot parameter. But on the downside, this will need
> > introducing additional parameter and also adding new restriction of
> > running CREATE-sub in txn  block for a specific case.
> >
> > 4. Don't alter replication in subscription DDLs. Instead, try to alter
> > replication slot's failover in the apply worker. This means we need to
> > execute alter_replication_slot each time before starting streaming in
> > the apply worker.
> >
> > This does not seem appealing to execute alter_replication_slot
> > everytime the apply worker starts. But if others think it as a better
> > option, it can be further analyzed.
>
> Thanks for describing, I also prefer 2, because it seems bit strange that
> CREATE statement leads ALTER.

Thanks for feedback.

> > Currently, our preference is option 2, as that looks a clean solution
> > and also aligns with ALTER-SUB behavior which is already documented.
> > Thoughts?
> >
> > --------------------------------
> > Note that we could not refer to the design of two_phase here, because
> > two_phase can be considered as a streaming option, so it's fine to
> > change the two_phase along with START_REPLICATION command. (the
> > two_phase is not changed in subscription DDLs, but get changed in
> > START_REPLICATION command). But the failover is closely related to a
> > replication slot itself.
> > --------------------------------

Sorry for causing confusion. This is not the statement which is
documented one, this was an additional note to support our analysis.

> Sorry, I cannot find statements. Where did you refer?

When I said that option 2 aligns with ALTER-SUB documented behaviour,
I meant the doc described in [1] stating "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 in the subscription...."

[1]: https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET

thanks
Shveta

On Thu, Apr 18, 2024 at 11:22:24AM +0530, Amit Kapila wrote:
> +1 for option 2 as it sounds logical to me and consistent with ALTER
> SUBSCRIPTION. BTW, IIUC, you are referring to: "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 in the subscription. When creating the slot, ensure the slot
> properties failover and two_phase match their counterpart parameters
> of the subscription." in docs [1], right?

FWIW, I'd also favor option 2, mostly on consistency ground as it
would offer a better user-experience.  On top of that, you're saying
that may lead to some simplifications in the CREATE path.  Without a
patch, it's hard to tell, though.

As far as I can see, this is not tracked as an open item and it should
be.  So I have added one.
--
Michael

On Thursday, April 18, 2024 1:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Hou,
> > >
> > > > Kuroda-San reported an issue off-list that:
> > > >
> > > > If user execute ALTER SUBSCRIPTION SET (failover) command inside a
> > > > txn block and rollback, only the subscription option change can be
> > > > rolled back, while the replication slot's failover change is preserved.
> > > >
> > > > This is because ALTER SUBSCRIPTION SET (failover) command
> > > > internally executes the replication command ALTER_REPLICATION_SLOT
> > > > to change the replication slot's failover property, but this
> > > > replication command execution cannot be rollback.
> > > >
> > > > To fix it, I think we can prevent user from executing ALTER
> > > > SUBSCRIPTION set
> > > > (failover) inside a txn block, which is also consistent to the
> > > > ALTER SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach
> a
> > > > small patch to address this.
> > >
> > > Thanks for posting the patch, the fix is same as my expectation.
> > > Also, should we add the restriction to the doc? I feel [1] can be updated.
> >
> > +1.
> >
> > Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
> > because we call alter_replication_slot in CREATE SUB as well, for the
> > case when slot_name is provided and create_slot=false. But the tricky
> > part is we call alter_replication_slot() when creating subscription
> > for both failover=true and false. That means if we want to fix it on
> > the similar line of ALTER SUB, we have to disallow user from executing
> > the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
> > to break some existing use cases. (previously, user can execute such a
> > command inside a txn block).
> >
> > So, we need to think if there are better ways to fix it.  After
> > discussion with Hou-San offlist, here are some ideas:
> >
> > 1. do not alter replication slot's failover option when CREATE
> > SUBSCRIPTION   WITH failover=false. This means we alter replication
> > slot only when failover is set to true. And thus we can disallow
> > CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
> > inside a txn block.
> >
> > This option allows user to run CREATE-SUB(create_slot=false) with
> > failover=false in txn block like earlier. But on the downside, it
> > makes the behavior inconsistent for otherwise simpler option like
> > failover,  i.e. with failover=true, CREATE SUB is not allowed in txn
> > block while with failover=false, it is allowed. It makes it slightly
> > complex to be understood by user.
> >
> > 2. let's not disallow CREATE SUB in txn block as earlier, just don't
> > perform internal alter-failover during CREATE SUB for existing
> > slots(create_slot=false, slot_name=xx)  i.e. when create_slot is
> > false, we will ignore failover parameter of CREATE SUB and it is
> > user's responsibility to set it appropriately using ALTER SUB cmd. For
> > create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
> >
> > This option does not add new restriction for CREATE SUB wrt txn block.
> > In context of failover with create_slot=false, we already have a
> > similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
> > SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
> > failover by himself. CREAT SUB can also be documented in similar way.
> > This seems simpler to be understood considering existing ALTER SUB's
> > behavior as well. Plus, this will make CREATE-SUB code slightly
> > simpler and thus easily manageable.
> >
> 
> +1 for option 2 as it sounds logical to me and consistent with ALTER
> SUBSCRIPTION.

+1.

Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
suggested. Since we don't connect pub to alter slot when (create_slot=false)
anymore, the restriction that disallows failover=true when connect=false is
also removed.

Best Regards,
Hou zj

Dear Shveta,

> When I said that option 2 aligns with ALTER-SUB documented behaviour,
> I meant the doc described in [1] stating "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 in the subscription...."
> 
> [1]:
> https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTER
> SUBSCRIPTION-PARAMS-SET

I see, thanks for the clarification. Agreed that the description is not conflict with
option 2.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Dear Hou,

Thanks for updating the patch! Let me confirm the content.

In your patch, the pg_dump.c was updated. IIUC the main reason was that
pg_restore executes some queries as single transactions so that ALTER
SUBSCRIPTION cannot be done, right?
Also, failover was synchronized only when we were in the upgrade mode, but
your patch seems to remove the condition. Can you clarify the reason?

Other than that, the patch LGTM.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/ 

Hi,

On Fri, Apr 19, 2024 at 12:39:40AM +0000, Zhijie Hou (Fujitsu) wrote:
> Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
> suggested. Since we don't connect pub to alter slot when (create_slot=false)
> anymore, the restriction that disallows failover=true when connect=false is
> also removed.

Thanks!

+          specified in the subscription. When creating the slot, ensure the slot
+          property <literal>failover</literal> matches the counterpart parameter
+          of the subscription.

The slot could be created before or after the subscription is created, so I think
it needs a bit of rewording (as here it sounds like the sub is already created),
, something like?

"Always ensure the slot property <literal>failover</literal> matches the
counterpart parameter of the subscription and vice versa."

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Apr 19, 2024 at 6:09 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
> suggested. Since we don't connect pub to alter slot when (create_slot=false)
> anymore, the restriction that disallows failover=true when connect=false is
> also removed.

Thanks for the patch. I feel getSubscription() also needs to get
'subfailover' option independent of dopt->binary_upgrade i.e. it needs
similar changes as that of dumpSubscription(). I tested pg_dump,
currently it is not dumping failover parameter for failover-enabled
subscriptions, perhaps due to the same bug.  Create-sub and Alter-sub
changes look good and work well.

thanks
Shveta

On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> In your patch, the pg_dump.c was updated. IIUC the main reason was that
> pg_restore executes some queries as single transactions so that ALTER
> SUBSCRIPTION cannot be done, right?

Yes, and please see below for other reasons.

> Also, failover was synchronized only when we were in the upgrade mode, but
> your patch seems to remove the condition. Can you clarify the reason?

We used ALTER SUBSCRIPTION in upgrade mode because it was not allowed to use
connect=false and failover=true together when CREATE SUBSCRIPTION. But since we
don't have this restriction anymore(we don't alter slot when creating sub
anymore), we can directly specify failover in CREATE SUBSCRIPTION and do that
in non-upgrade mode as well.

Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.

[1] https://www.postgresql.org/message-id/CAJpy0uD3YOeDg-tTCUi3EZ8vznRDfDqO_k6LepJpXUV1Z_%3DgkA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/ZiIxuaiINsuaWuDK%40ip-10-97-1-34.eu-west-3.compute.internal

Best Regards,
Hou zj

On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> wrote:
> > In your patch, the pg_dump.c was updated. IIUC the main reason was that
> > pg_restore executes some queries as single transactions so that ALTER
> > SUBSCRIPTION cannot be done, right?
>
> Yes, and please see below for other reasons.
>
> > Also, failover was synchronized only when we were in the upgrade mode, but
> > your patch seems to remove the condition. Can you clarify the reason?
>
> We used ALTER SUBSCRIPTION in upgrade mode because it was not allowed to use
> connect=false and failover=true together when CREATE SUBSCRIPTION. But since we
> don't have this restriction anymore(we don't alter slot when creating sub
> anymore), we can directly specify failover in CREATE SUBSCRIPTION and do that
> in non-upgrade mode as well.
>
> Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.

 Tested the patch, works well.

thanks
Shveta

Hi,

On Mon, Apr 22, 2024 at 11:32:14AM +0530, shveta malik wrote:
> On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> > Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.

Thanks!

>  Tested the patch, works well.

Same here.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Apr 22, 2024 at 2:31 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Mon, Apr 22, 2024 at 11:32:14AM +0530, shveta malik wrote:
> > On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > > Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.
>
> Thanks!
>
> >  Tested the patch, works well.
>
> Same here.
>

Pushed!

--
With Regards,
Amit Kapila.