Re: Disallow changing slot's failover option in transaction block

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.