Re: Slow catchup of 2PC (twophase) transactions on replica in LR - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Slow catchup of 2PC (twophase) transactions on replica in LR
Date
Msg-id CAA4eK1J85P26-DtRyGn3o8uTZzrir=6FiNg-HZuTprgCCjGpJw@mail.gmail.com
Whole thread Raw
In response to Re: Slow catchup of 2PC (twophase) transactions on replica in LR  (Peter Smith <smithpb2250@gmail.com>)
Responses Re: Slow catchup of 2PC (twophase) transactions on replica in LR
List pgsql-hackers
On Fri, Jul 19, 2024 at 8:06 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ======
> You wrote "tried to make the two_phase change before failover option
> wherever it makes sense to keep the code consistent". But, still
> failover is coded first in lots of places:
> - libpqrcv_alter_slot
> - ReplicationSlotAlter
> - AlterReplicationSlot
> etc.
>

In ReplicationSlotAlter(), there are error conditions related to
standby and failover slots which are better checked before setting
two_phase property. The main reason for keeping two_phase before the
failover option in subscriptioncmds.c is that SUBOPT_TWOPHASE_COMMIT
was introduced before the equivalent failover option. We can do at
other places as you pointed but I didn't see any compelling reason to
not do what we normally do which is to add the new options at the end.

> ======
> src/include/replication/slot.h
>
> 1.
> -extern void ReplicationSlotAlter(const char *name, bool failover);
> +extern void ReplicationSlotAlter(const char *name, bool *failover,
> + bool *two_phase);
>
> Use const?
>

If so, we need to use const both for failover and two_phase but not
sure if that is required here. We can evaluate that separately if
required by comparing it with similar instances.

> ======
> 99.
> Please see attached diffs implementing the nitpicks mentioned above
>

These look good to me, so will incorporate them in the next patch.

--
With Regards,
Amit Kapila.



pgsql-hackers by date:

Previous
From: Peter Smith
Date:
Subject: Re: walsender.c fileheader comment
Next
From: Ashutosh Bapat
Date:
Subject: Re: Add mention of execution time memory for enable_partitionwise_* GUCs