Re: Allow logical failover slots to wait on synchronous replication

Hi Amit,

On Mon, Aug 26, 2024 at 11:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I wanted a simple test where in the first case you use
> synchronous_standby_names = 'ANY 3 (A,B,C,D,E)' and in the second case
> use standby_slot_names = A_slot, B_slot, C_slot, D_slot, E_slot. You
> can try some variations of it as well. The idea is that even if the
> performance is less for synchronous_standby_names configuration, we
> should be able to document it. This will help users to decide what is
> ...
> What is the difference between "Test failover_slots with
> synchronized_standby_slots = 'rr_1, rr_2,
> > rr_3, rr_4, rr_5'" and "Test failover_slots waiting on sync_rep no new shared cache"? I want to know what configuration did you used for synchronous_standby_names in the latter case.

Sorry for the confusion due to the bad-naming of the test cases, let
me rephrase.
All three tests had synchronous_standby_names = 'ANY 3 (A,B,C,D,E)'
set with synchronous_commit = 'on', and failover_slots = 'on'
for the 10 logical slots.

# Test failover_slots with synchronized_standby_slots = 'rr_1, rr_2,
rr_3, rr_4, rr_5'
This is the test you wanted where the logical clients are waiting on
all 5 slots to acknowledge the change since
'synchronized_standby_slots' takes priority when set.

# Test failover_slots sync rep no cache
This test has 'synchronized_standby_slots' commented out, and without
relying on the new cache introduced in 0003.
Logical clients will wait on synchronous_standby_names in this case.

# Test failover slots with additional shared cache
This test also has 'synchronized_standby_slots' commented out, and
logical clients will wait on the LSNs
reported from synchronous_standby_names but it relies on a new cache
to reduce contention on SyncRepLock.

> The idea is that even if the
> performance is less for synchronous_standby_names configuration, we
> should be able to document it. This will help users to decide what is
> best for them.

Makes sense.

> I am also not sure especially as the test results didn't shown much
> improvement and the code also becomes bit complicated. BTW, in the
> 0003 version in the below code:

That's fair, I've updated to be more in line with 0002.

> + /* Cache values to reduce contention */
> + LWLockAcquire(SyncRepLock, LW_SHARED);
> + memcpy((XLogRecPtr *) walsndctl->cached_lsn, (XLogRecPtr *)
> walsndctl->lsn, sizeof(lsn));
> + LWLockRelease(SyncRepLock);
>
> Which mode lsn is being copied? I am not sure if I understood this
> part of the code.

All of the mode LSNs are being copied in case SyncRepWaitMode changes in
the next iteration. I've removed that part but kept:

> + memcpy(lsn, (XLogRecPtr *) walsndctl->lsn, sizeof(lsn));

as suggested by Bertrand to avoid the for loop updating values one-by-one.

Here's what's logged after the memcpy:

2024-08-28 19:41:13.798 UTC [1160413] LOG: lsn[0] after memcpy is: 279/752C7FF0
2024-08-28 19:41:13.798 UTC [1160413] LOG: lsn[1] after memcpy is: 279/752C7F20
2024-08-28 19:41:13.798 UTC [1160413] LOG: lsn[2] after memcpy is: 279/752C7F20

> In the 0002 version, in the following code [1], you are referring to
> LSN mode which is enabled for logical walsender irrespective of the
> mode used by the physical walsender. It is possible that they are
> always the same but that is not evident from the code or comments in
> the patch.

They are almost always the same, I tried to indicate that with the
following comment in the patch, but I could make it more explicit?
> /* Initialize value in case SIGHUP changing to SYNC_REP_NO_WAIT */

At the beginning we set

> int mode = SyncRepWaitMode;

At this time, the logical walsender mode it's checking against is the
same as what the physical walsenders are using.
It's possible that this mode is no longer the same when we execute the
following check:

> if (lsn[mode] >= wait_for_lsn)

because of a SIGHUP to synchronous_commit that changes SyncRepWaitMode
to some other value

We cache the value instead of
> if (lsn[SyncRepWaitMode] >= wait_for_lsn)

because SYNC_REP_NO_WAIT is -1. If SyncRepWaitMode is set to this it
leads to out of bounds access.

I've attached a new patch that removes the shared cache introduced in 0003.

Thanks,
--
John Hsu - Amazon Web Services