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

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>

To: 'Peter Smith' <smithpb2250@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>

Cc: Amit Kapila <amit.kapila16@gmail.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Vitaly Davydov <v.davydov@postgrespro.ru>, Ajin Cherian <itsajin@gmail.com>

Date: 2024-07-17T05:13:35Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Fix random failure in 021_twophase.
- 0dcea330babd 18.0 landed
Allow altering of two_phase option of a SUBSCRIPTION.
- 1462aad2e447 18.0 landed
Doc: use true|false rather than on|off for "failover" option
- fa65a022db26 17.0 cited
Support an optional asynchronous commit mode, in which we don't flush WAL
- 4a78cdeb6b59 8.3.0 cited

Attachments

v19-0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIP.patch (application/octet-stream) patch v19-0001
v19-0002-Alter-slot-option-two_phase-only-when-altering-t.patch (application/octet-stream) patch v19-0002
v19-0003-Notify-users-to-roll-back-prepared-transactions.patch (application/octet-stream) patch v19-0003

Dear Hou, Peter,

Thanks for giving comments! PSA new version.
Almost comments were addressed.
What's new:
0001 - IsTwoPhaseTransactionGidForSubid() was updated per comment from Hou-san [1].
          Some nitpicks were accepted.
0002 - An argument in CheckAlterSubOption() was renamed to " slot_needs_update "
          Some nitpicks were accepted.
0003 - Some nitpicks were accepted.

Below part contains the reason why I rejected some comments.

> CommonChecksForFailoverAndTwophase:
> nitpick - added Assert for the generic-looking "option" parameter name

The style looks strange for me, using multiple strcmp() is more straightforward.
Added like that.

> 1c.
> If the error checks can be moved to be done up-front, then all the 'needs_update'
> can be combined. Avoiding multiple checks to 'needs_update' will make this function simpler.

This style was needed to preserve error condition for failover option. Not changed.

[1]: https://www.postgresql.org/message-id/OS3PR01MB571834FBD3E6D3804484038F94A32%40OS3PR01MB5718.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED