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

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

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