Subject: Re: Allow logical failover slots to wait on synchronous replication

From: John H <[email-masked]>
Date: Mon, 8 Jul 2024 12:12:13 -0700

Hi Amit,

Thanks for taking a look.

On Tue, Jun 18, 2024 at 10:34 PM Amit Kapila <[email-masked]> wrote:

>
> The reading indicates when you set 'standby_slot_names_from_syncrep',
> the TPS reduces as compared to when it is not set. It would be better
> to see the data comparing 'standby_slot_names_from_syncrep' and the
> existing parameter 'standby_slot_names'.

I added new benchmark numbers in the reply to Bertrand, but I'll
include in this thread for posterity.

Writer with 5 synchronous replicas, 10 pg_recvlogical clients and
pgbench all running from the same server.
Command: pgbench -c 4 -j 4 -T 600 -U "ec2-user" -d postgres -r -P 5

Result with: standby_slot_names =
'replica_1,replica_2,replica_3,replica_4,replica_5'

latency average = 5.600 ms
latency stddev = 2.854 ms
initial connection time = 5.503 ms
tps = 714.148263 (without initial connection time)

Result with: standby_slot_names_from_syncrep = 'true',
synchronous_standby_names = 'ANY 3 (A,B,C,D,E)'

latency average = 5.740 ms
latency stddev = 2.543 ms
initial connection time = 4.093 ms
tps = 696.776249 (without initial connection time)

Result with nothing set:

latency average = 5.090 ms
latency stddev = 3.467 ms
initial connection time = 4.989 ms
tps = 785.665963 (without initial connection time)

> Can we make it a default
> behavior that logical slots marked with a failover option will wait
> for 'synchronous_standby_names' as per your patch's idea unless
> 'standby_slot_names' is specified? I don't know if there is any value
> in setting the 'failover' option for a slot without specifying
> 'standby_slot_names', so was wondering if we can additionally tie it
> to 'synchronous_standby_names'. Any better ideas?
>

No, I think that works pretty cleanly actually. Reserving some special
keyword isn't great
which is the only other thing I can think of. I've updated the patch
and tests to reflect that.

Attached the patch that addresses these changes.

-- 
John Hsu - Amazon Web Services

---

From: shveta malik <[email-masked]>
Date: Thu, 18 Jul 2024 09:25:37 +0530

> Attached the patch that addresses these changes.

Thank You for the patch. I like the overall idea, it is a useful one
and will make user experience better. Please find few comments.

1)
I agree with the idea that instead of introducing new GUC, we can make
failover logical slots wait on 'synchronous_standby_names', but this
will leave user no option to unlink failover-enabled logical
subscribers from the wait on synchronous standbys. We provide user a
way to switch off automatic slot-sync by disabling
'sync_replication_slots' and we also provide user a way to manually
sync the slots using function 'pg_sync_replication_slots()' and
nowhere we make it mandatory to set 'synchronized_standby_slots'; in
fact in docs, it is a recommended setting and not a mandatory one.
User can always create failover slots, switch off automatic slot sync
and disable wait on standbys by not specifying
'synchronized_standby_slots' and do the slot-sync and consistency
checks manually when needed. I feel, for worst case scenarios, we
should provide user an option to delink failover-enabled logical
subscriptions from  'synchronous_standby_names'.

We can have 'synchronized_standby_slots' (standby_slot_names) to
accept one such option as in 'SAME_AS_SYNCREP_STANDBYS' or  say
'DEFAULT'. So when 'synchronous_standby_names' is comma separated
list, we pick those slots; if it is empty, then no wait on standbys,
and if its value is 'DEFAULT' as configured by user, then go with
'synchronous_standby_names'. Thoughts?

2)
When  'synchronized_standby_slots' is configured but standby named in
it is down blocking logical replication, then we get a WARNING in
subscriber's log file:

WARNING:  replication slot "standby_2" specified in parameter
synchronized_standby_slots does not have active_pid.
DETAIL:  Logical replication is waiting on the standby associated with
"standby_2".
HINT:  Consider starting standby associated with "standby_2" or amend
parameter synchronized_standby_slots.

But OTOH, when  'synchronous_standby_names' is configured instead of
'synchronized_standby_slots' and any of the standbys listed is down
blocking logical replication, we do not get any sort of warning. It is
inconsistent behavior. Also user might be left clueless on why
subscribers are not getting changes.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 18 Jul 2024 09:57:17 +0530

On Thu, Jul 18, 2024 at 9:25 AM shveta malik <[email-masked]> wrote:
> and if its value is 'DEFAULT' as configured by the user, then go with

One correction here
('synchronous_standby_names-->synchronized_standby_slots). Corrected
version:

So when 'synchronized_standby_slots' is comma separated list, we pick
those slots; if it is empty, then no wait on standbys, and if its
value is 'DEFAULT' as configured by user, then go with
'synchronous_standby_names'. Thoughts?

---

From: John H <[email-masked]>
Date: Thu, 18 Jul 2024 14:21:59 -0700

Hi Shveta,

Thanks for taking a look at the patch.

> > subscribers from the wait on synchronous standbys.

That's a good point. I'm a bit biased in that I don't think there's a
great reason why someone would
want to replicate logical changes out of the synchronous cluster
without it having been synchronously replicated
 but yes this would be different behavior compared to strictly the slot one.

> ...

I think I'd prefer having a separate GUC if the alternative is to reserve
special keywords in 'synchronized_standby_slots' but I'm not sure if I
feel strongly about that.

> > subscribers are not getting changes.

Ah that's a gap. Let me add some logging/warning in a similar fashion.
Although I think I'd have the warning be relatively generic (e.g.
changes are blocked because
they're not synchronously committed)

Thanks,

---

From: shveta malik <[email-masked]>
Date: Mon, 22 Jul 2024 09:11:48 +0530

> feel strongly about that.

My only concern is, earlier we provided a way to set the failover
property of slots even without mandatorily wait on physical standbys.
But now we will be changing this behaviour. Okay, we can see what
other comments. If we plan to go this way, we can change docs to
clearly mention this.

> they're not synchronously committed)

okay, sounds good.

thanks
Shveta

---

From: John H <[email-masked]>
Date: Mon, 26 Aug 2024 12:25:51 -0700

Hi Shveta,

> > they're not synchronously committed)

I took a look at having similar warnings the existing
'synchronized_standby_slots' feature has
and I don't think it's particularly feasible.

The checks/warnings for 'synchronized_standby_slots' are intended to
protect against misconfiguration.
They consist of slot validation (valid slot_name, not logical slot,
slot has not been invalidated), and
whether or not the slot is active.

I don't think there's a "misconfiguration" equivalent for waiting on
synchronous_commit.
With the current proposal, once you have (synchronous_commit enabled
&& failover_slots), logical
decoding is dependent on whether or not the writes have been
replicated to a synchronous replica.
If there is no data being replicated out of the logical slot, it is
because from the perspective of the
database no writes have been committed yet. I don't think it would
make sense to add logging/warning as to
why a transaction is still not committed (e.g. which potential replica
is the one lagging). There isn't a
nice way to determine why synchronous commit is waiting without being
particularly invasive, and even then
it could be particularly noisy (e.g. provide all the application_names
that we are potentially waiting on).

Thanks,

---

From: shveta malik <[email-masked]>
Date: Wed, 28 Aug 2024 08:46:21 +0530

> that we are potentially waiting on).

Okay. Thanks for the details. I see your point. I will review to see
if anything comes to my mind for a simpler way to do this.

thanks
Shveta