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

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Apr 2024 11:22:24 +0530

On Tue, Apr 16, 2024 at 5:06 PM shveta malik <[email-masked]> wrote:
>
> On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
> <[email-masked]> 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.

---

From: shveta malik <[email-masked]>
Date: Thu, 18 Apr 2024 11:39:41 +0530

> of the subscription." in docs [1], right?

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

thanks
Shveta

---

From: Michael Paquier <[email-masked]>
Date: Fri, 19 Apr 2024 08:34:49 +0900

> 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

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 19 Apr 2024 00:39:40 +0000

On Thursday, April 18, 2024 1:52 PM Amit Kapila <[email-masked]> wrote:
> > > > 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.
> > > >
> > > > (failover) inside a txn block, which is also consistent to the
> > > > ALTER SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach
> a
> > > > small patch to address this.
> > >
> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 19 Apr 2024 02:54:17 +0000

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/

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 19 Apr 2024 08:56:25 +0000

Hi,

> anymore, the restriction that disallows failover=true when connect=false is

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

---

From: shveta malik <[email-masked]>
Date: Fri, 19 Apr 2024 15:44:38 +0530

> anymore, the restriction that disallows failover=true when connect=false is

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

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 22 Apr 2024 00:27:42 +0000

> SUBSCRIPTION cannot be done, right?

Yes, and please see below for other reasons.

> 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