Horton

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

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, 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-17T23:24:29Z
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 →

  1. Fix random failure in 021_twophase.

  2. Allow altering of two_phase option of a SUBSCRIPTION.

  3. Doc: use true|false rather than on|off for "failover" option

  4. Support an optional asynchronous commit mode, in which we don't flush WAL

Attachments

Hi, here are my review comments for patch v19-0002.

======
src/backend/commands/subscriptioncmds.c

CheckAlterSubOption:
nitpick - tweak some comment wording

~

On Wed, Jul 17, 2024 at 3:13 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > 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.
>

nitpick - Hmm. I think you might be trying to preserve the ordering of
errors when that order is of no consequence. AFAICT which error comes
first here is neither documented nor regression tested. e.g.
reordering them gives no problem for testing, but OTOH reordering them
does simplify the code. Anyway, I have modified the code in my
attached nitpicks diffs to demonstrate this suggestion in case you
change your mind.

~~~

AlterSubscription:
nitpick - let's keep all the variables called 'update_xxx' together.
nitpick - comment typo /needs/need/
nitpick - tweak some comment wording

======
src/test/subscription/t/021_twophase.pl

nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.

======
Kind Regards,
Peter Smith.
Fujitsu Australia