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

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 5 Jul 2024 05:39:05 +0000

Dear Amit,

Thanks for giving comments. I hope all comments have been addressed.
PSA new version.

> > 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);
> > ```
> >
> 
> 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.

I used the function because it internally waits until all workers are exited.
But OK, I modified like you suggested (logicalrep_workers_find() is used).

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 5 Jul 2024 09:08:17 +0000

Dear Amit,

Sorry, I forgot to say one content.

> But OK, I modified like you suggested (logicalrep_workers_find() is used).

Based on the reason, after the above modification, test codes prior to v14
sometimes failed because backend could execute ALTER SUBSCRIPTION ... SET (two_phase).
So I added lines in test codes to poll until workers are exited, e.g.,

```
+# Alter subscription two_phase to false
+$node_subscriber->safe_psql('postgres',
+    "ALTER SUBSCRIPTION tap_sub_copy DISABLE;");
+$node_subscriber->poll_query_until('postgres',
+    "SELECT count(*) = 0 FROM pg_stat_activity WHERE backend_type = 'logical replication worker'"
+);
+$node_subscriber->safe_psql(
+       'postgres', "
+    ALTER SUBSCRIPTION tap_sub_copy SET (two_phase = false);
+    ALTER SUBSCRIPTION tap_sub_copy ENABLE;");
```

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

---

From: "Vitaly Davydov" <[email-masked]>
Date: Fri, 05 Jul 2024 17:06:19 +0300

Hi Kuroda-san,

Thank you very much for the patch. In general, it seem to work well for me, but there seems to be a memory access problem in libpqrcv_alter_slot -> quote_identifier in case of NULL slot_name. It happens, if the two_phase option is altered on a subscription without slot. I think, a simple check for NULL may fix the problem. I guess, the same problem may be for failover option.

Another possible problem is related to my use case. I haven't reproduced this case, just some thoughts. I guess, when two_phase is ON, the PREPARE statement may be truncated from the WAL at checkpoint, but COMMIT PREPARED is still kept in the WAL. On catchup, I would ask the master to send transactions from some restart LSN. I would like to get all such transactions competely, with theirs bodies, not only COMMIT PREPARED messages. One of the solutions is to have an option for the slot to keep the WAL like with two_phase = OFF independently on its two_phase option. It is just an idea.

With best regards,
Vitaly