Subject: RE: Slow catchup of 2PC (twophase) transactions on replica in LR

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 15 Apr 2024 07:57:49 +0000

Dear Amit,

> Vitaly, does the minimal solution provided by the proposed patch
> (Allow to alter two_phase option of a subscriber provided no
> uncommitted
> prepared transactions are pending on that subscription.) address your use case?

I think we do not have to handle cases which there are prepared transactions on
publisher/subscriber, as the first step. It leads additional complexity and we
do not have smarter solutions, especially for problem 2.
IIUC it meets the Vitaly's condition, right?

> > 1. While toggling two_phase from true to false, we could probably get a list of
> prepared transactions for this subscriber id and rollback/abort the prepared
> transactions. This will allow the transactions to be re-applied like a normal
> transaction when the commit comes. Alternatively, if this isn't appropriate doing it
> in the ALTER SUBSCRIPTION context, we could store the xids of all prepared
> transactions of this subscription in a list and when the corresponding xid is being
> committed by the apply worker, prior to commit, we make sure the previously
> prepared transaction is rolled back. But this would add the overhead of checking
> this list every time a transaction is committed by the apply worker.
> >
> 
> In the second solution, if you check at the time of commit whether
> there exists a prior prepared transaction then won't we end up
> applying the changes twice? I think we can first try to achieve it at
> the time of Alter Subscription because the other solution can add
> overhead at each commit?

Yeah, at least the second solution might be problematic. I prototyped
the first one and worked well. However, to make the feature more consistent,
it is prohibit to exist prepared transactions on subscriber for now.
We can ease based on the requirement.

> > 2. No solution yet.
> >
> 
> One naive idea is that on the publisher we can remember whether the
> prepare has been sent and if so then only send commit_prepared,
> otherwise send the entire transaction. On the subscriber-side, we
> somehow, need to ensure before applying the first change whether the
> corresponding transaction is already prepared and if so then skip the
> changes and just perform the commit prepared. One drawback of this
> approach is that after restart, the prepare flag wouldn't be saved in
> the memory and we end up sending the entire transaction again. One way
> to avoid this overhead is that the publisher before sending the entire
> transaction checks with subscriber whether it has a prepared
> transaction corresponding to the current commit. I understand that
> this is not a good idea even if it works but I don't have any better
> ideas. What do you think?

I considered but not sure it is good to add such mechanism. Your idea requires
additional wait-loop, which might lead bugs and unexpected behavior. And it may
degrade the performance based on the network environment.
As for the another solution (worker sends a list of prepared transactions), it
is also not so good because list of prepared transactions may be huge.

Based on above, I think we can reject the case for now.

FYI - We also considered the idea which walsender waits until all prepared transactions
are resolved before decoding and sending changes, but it did not work well
- the restarted walsender sent only COMMIT PREPARED record for transactions which
have been prepared before disabling the subscription. This happened because
1) if the two_phase option of slots is false, the confirmed_flush can be ahead of
   PREPARE record, and
2) after the altering and restarting, start_decoding_at becomes same as
   confirmed_flush and records behind this won't be decoded.

> > 3. We could mandate that the altering of two_phase state only be done after
> disabling the subscription, just like how it is handled for failover option.
> >
> 
> makes sense.

OK, this spec was added.

According to above, I updated the patch with Ajin.
0001 - extends ALTER SUBSCRIPTION statement. A tab-completion was added.
0002 - mandates the subscription has been disabled. Since no need to change 
       AtEOXact_ApplyLauncher(), the change is reverted.
       If no objections, this can be included to 0001.
0003 - checks whether there are transactions prepared by the worker. If found,
       rejects the ALTER SUBSCRIPTION command.
0004 - checks whether there are transactions prepared on publisher. The backend
       connects to the publisher and confirms it. If found, rejects the ALTER
       SUBSCRIPTION command.
0005 - adds TAP test for it.

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

---

From: Amit Kapila <[email-masked]>
Date: Mon, 15 Apr 2024 15:16:53 +0530

>    confirmed_flush and records behind this won't be decoded.

I don't understand the exact problem you are facing. IIUC, if the
commit is after start_decoding_at point and prepare was before it, we
expect to send the entire transaction followed by a commit record. The
restart_lsn should be before the start of such a transaction and we
should have recorded the changes in the reorder buffer.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 16 Apr 2024 02:18:29 +0000

Dear Amit,

> should have recorded the changes in the reorder buffer.

This behavior is right for two_phase = false case. But if the parameter is
altered between PREPARE and COMMIT PREPARED, there is a possibility that only
COMMIT PREPARED is sent. As the first place, the executed workload is below.

1. created a subscription with (two_phase = false)
2. prepared a transaction on publisher
3. disabled the subscription once
4. altered the subscription to two_phase = true
5. enabled the subscription again
6. did COMMIT PREPARED on the publisher

-> Apply worker would raise an ERROR while applying COMMIT PREPARED record:
ERROR:  prepared transaction with identifier "pg_gid_XXX_YYY" does not exist

Below part describes why the ERROR occurred.

======

### Regarding 1) the confirmed_flush can be ahead of PREPARE record,

If two_phase is off, as you might know, confirmed_flush can be ahead of PREPARE
record by keepalive mechanism.

Walsender sometimes sends a keepalive message in WalSndKeepalive(). Here the LSN
is written, which is lastly decoded record. Since the PREPARE record is skipped
(just handled by ReorderBufferProcessXid()), sometimes the written LSN in the
message can be ahead of PREPARE record. If the WAL records are aligned like below,
the LSN can point CHECKPOINT_ONLINE.

...
INSERT
PREPARE txn1
CHECKPOINT_ONLINE
...

On worker side, when it receives the keepalive, it compares the LSN in the
message and lastly received LSN, and advance last_received. Then, the worker replies
to the walsender, and at that time it replies that last_recevied record has been
flushed on the subscriber. See send_feedback().
 
On publisher, when the walsender receives the message from subscriber, it reads
the message and advance the confirmed_flush to the written value. If the walsender
sends LSN which locates ahead PREPARE, the confirmed flush is updated as well.

### Regarding 2) after the altering, records behind the confirmed_flush are not decoded

Then, at decoding phase. The snapshot builder determines the point where decoding
is resumed, as start_decoding_at. After the restart, the value is same as
confirmed_flush of the slot. Since the confiremed_fluish is ahead of PREPARE,
the start_decoding_at becomes ahead as well, so whole of prepared transactions
are not decoded.

======

Attached zip file contains the PoC and used script. You can refer what I really did.

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 16 Apr 2024 11:55:11 +0530

On Tue, Apr 16, 2024 at 7:48 AM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> COMMIT PREPARED is sent.

Can you please once consider the idea shared by me at [1] (One naive
idea is that on the publisher .....) to solve this problem?

[1] - https://www.postgresql.org/message-id/CAA4eK1K1fSkeK%3Dkc26G5cq87vQG4%3D1qs_b%2Bno4%2Bep654SeBy1w%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Ajin Cherian <[email-masked]>
Date: Thu, 18 Apr 2024 16:26:22 +1000

On Tue, Apr 16, 2024 at 4:25 PM Amit Kapila <[email-masked]> wrote:

> [1] -
> https://www.postgresql.org/message-id/CAA4eK1K1fSkeK%3Dkc26G5cq87vQG4%3D1qs_b%2Bno4%2Bep654SeBy1w%40mail.gmail.com
>
>
>
Expanding on Amit's idea, we found out that there is already a mechanism in
code to fully decode prepared transactions prior to a defined LSN where
two_phase is enabled using the "two_phase_at" LSN in the slot. Look at
ReorderBufferFinishPrepared() on how this is done. This code was not
working as expected in our patch because
we were setting two_phase on the slot to true as soon as the alter command
was received. This was not the correct way, initially when two_phase is
enabled, the two_phase changes to pending state and two_phase option on the
slot should only be set to true when two_phase moves from pending to
enabled. This will happen once the replication is restarted with two_phase
option. Look at code in  CreateDecodingContext() on how "two_phase_at" is
set in the slot when done this way. So we changed the code to not remotely
alter two_phase when toggling from false to true. With this change, now
even if there are pending transactions on the publisher when toggling
two_phase from false to true, these pending transactions will be fully
decoded and sent once the commit prepared is decoded as the pending
prepared transactions are prior to the "two_phase_at" LSN. With this patch,
now we are able to handle both pending prepared transactions when altering
two_phase from true to false as well as false to true.

Attaching the patch for your review and comments. Big thanks to Kuroda-san
for also working on the patch.

regards,
Ajin Cherian
Fujitsu Australia.

---

From: Ajin Cherian <[email-masked]>
Date: Mon, 22 Apr 2024 17:34:58 +1000

> for also working on the patch.
Looking at this a bit more, maybe rolling back all prepared transactions on
the subscriber when toggling two_phase from true to false might not be
desirable for the customer. Maybe we should have an option for customers to
control whether transactions should be rolled back or not. Maybe
transactions should only be rolled back if a "force" option is also set.
What do people think?

regards,
Ajin Cherian
Fujitsu Australia

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 22 Apr 2024 08:56:42 +0000

Dear hackers,

> Looking at this a bit more, maybe rolling back all prepared transactions on the
> subscriber when toggling two_phase from true to false might not be desirable
> for the customer. Maybe we should have an option for customers to control
> whether transactions should be rolled back or not. Maybe transactions should
> only be rolled back if a "force" option is also set. What do people think?

And here is a patch for adds new option "force_alter" (better name is very welcome).
It could be used only when altering two_phase option. Let me share examples.

Assuming that there are logical replication system with two_phase = on, and
there are prepared transactions:

```
subscriber=# SELECT * FROM pg_prepared_xacts ;
 transaction |       gid        |           prepared            |  owner   | database 
-------------+------------------+-------------------------------+----------+----------
         741 | pg_gid_16390_741 | 2024-04-22 08:02:34.727913+00 | postgres | postgres
         742 | pg_gid_16390_742 | 2024-04-22 08:02:34.729486+00 | postgres | postgres
(2 rows)
```

At that time, altering two_phase to false alone will be failed:

```
subscriber=# ALTER SUBSCRIPTION sub DISABLE ;
ALTER SUBSCRIPTION
subscriber=# ALTER SUBSCRIPTION sub SET (two_phase = off);
ERROR:  cannot alter two_phase = false when there are prepared transactions
```

It succeeds if force_alter is also expressly set. Prepared transactions will be
aborted at that time.

```
subscriber=# ALTER SUBSCRIPTION sub SET (two_phase = off, force_alter = on);
ALTER SUBSCRIPTION
subscriber=# SELECT * FROM pg_prepared_xacts ;
 transaction | gid | prepared | owner | database 
-------------+-----+----------+-------+----------
(0 rows)
```

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Jul 2024 12:25:18 +0530

On Mon, Apr 22, 2024 at 2:26 PM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
 ```
> subscriber=# ALTER SUBSCRIPTION sub SET (two_phase = off, force_alter = on);

Isn't it better to give a Notice when force_alter option leads to the
rollback of already prepared transactions?

I have another question on the latest 0001 patch:
+ /*
+ * Stop all the subscription workers, just in case.
+ * Workers may still survive even if the subscription is
+ * disabled.
+ */
+ logicalrep_workers_stop(subid);

In which case the workers will survive when the subscription is disabled?

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 4 Jul 2024 08:04:47 +0000

Dear Amit,

> rollback of already prepared transactions?

Indeed. I think this can be added for 0003. For now, it says like:

```
postgres=# ALTER SUBSCRIPTION sub SET (TWO_PHASE = off, FORCE_ALTER = on);
WARNING:  requested altering to two_phase = false but there are prepared transactions done by the subscription
DETAIL:  Such transactions are being rollbacked.
ALTER SUBSCRIPTION
```

> In which case the workers will survive when the subscription is disabled?

I think both normal and tablesync worker can survive, because ALTER SUBSCRIPTION
DISABLE command does not send signal to workers. It just change the system catalog.
logicalrep_workers_stop() is added to ensure all workers are stopped.

Actually, earlier version (-v3) did not have a mechanism but they sometimes got
assertion failures in maybe_reread_subscription(). This was because the survived
workers read pg_subscription catalog and failed below assertion:

```
	/* two-phase cannot be altered while the worker exists */
	Assert(newsub->twophasestate == MySubscription->twophasestate);
```

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 4 Jul 2024 17:01:40 +0530

> DETAIL:  Such transactions are being rollbacked.

Is it possible to get a NOTICE instead of a WARNING?

>         Assert(newsub->twophasestate == MySubscription->twophasestate);

But that is not a good reason for this operation to stop workers
first. Instead, we should prohibit this operation if any worker is
present. The reason is that there is always a chance that if any
worker is alive, it can prepare a new transaction after we have
checked for the presence of any prepared transactions.

Comments:
=========
1.
There is no need to do something remarkable regarding
+ * the "false" to "true" case; the backend process alters
+ * subtwophase <funny_char> to LOGICALREP_TWOPHASE_STATE_PENDING once.
+ * After the subscription is enabled, a new logical
+ * replication worker requests to change the two_phase
+ * option of its slot from pending to true when the
+ * initial data synchronization is done. The code path is
+ * the same as the case in which two_phase <funny_char> is initially
+ * set <funny_char> to true.

The patch has some funny characters in the above comment at the places
highlighted by me. It seems you have copied from some editor that has
inserted such characters.

2.
/*
* Do not allow toggling of two_phase option. Doing so could cause
* missing of transactions and lead to an inconsistent replica.
* See comments atop worker.c
*
* Note: Unsupported twophase indicates that this call originated
* from AlterSubscription.
*/
if (!IsSet(supported_opts, SUBOPT_TWOPHASE_COMMIT))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("unrecognized subscription parameter: \"%s\"", defel->defname)));

This part of the code must either be removed or converted to an assert.

3. The tests added in 099_twophase_added.pl should be part of 021_twophase.pl

-- 
With Regards,
Amit Kapila.