Re: Disallow changing slot's failover option in transaction block - Mailing list pgsql-hackers

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