Subject: Allow async standbys wait for sync replication (was: Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers)

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 25 Feb 2022 20:31:37 +0530

On Thu, Jan 6, 2022 at 1:29 PM SATYANARAYANA NARLAPURAM
<[email-masked]> wrote:
>
> Consider a cluster formation where we have a Primary(P), Sync Replica(S1), and multiple async replicas for disaster recovery and read scaling (within the region and outside the region). In this setup, S1 is the preferred failover target in an event of the primary failure. When a transaction is committed on the primary, it is not acknowledged to the client until the primary gets an acknowledgment from the sync standby that the WAL is flushed to the disk (assume synchrnous_commit configuration is remote_flush). However, walsenders corresponds to the async replica on the primary don't wait for the flush acknowledgment from the primary and send the WAL to the async standbys (also any logical replication/decoding clients). So it is possible for the async replicas and logical client ahead of the sync replica. If a failover is initiated in such a scenario, to bring the formation into a healthy state we have to either
>
>  run the pg_rewind on the async replicas for them to reconnect with the new primary or
> collect the latest WAL across the replicas and feed the standby.
>
> Both these operations are involved, error prone, and can cause multiple minutes of downtime if done manually. In addition, there is a window where the async replicas can show the data that was neither acknowledged to the client nor committed on standby. Logical clients if they are ahead may need to reseed the data as no easy rewind option for them.
>
> I would like to propose a GUC send_Wal_after_quorum_committed which when set to ON, walsenders corresponds to async standbys and logical replication workers wait until the LSN is quorum committed on the primary before sending it to the standby. This not only simplifies the post failover steps but avoids unnecessary downtime for the async replicas. Thoughts?

Thanks Satya and others for the inputs. Here's the v1 patch that
basically allows async wal senders to wait until the sync standbys
report their flush lsn back to the primary. Please let me know your
thoughts.

I've done pgbench testing to see if the patch causes any problems. I
ran tests two times, there isn't much difference in the txns per
seconds (tps), although there's a delay in the async standby receiving
the WAL, after all, that's the feature we are pursuing.

[1]
HEAD or WITHOUT PATCH:
./pgbench -c 10 -t 500 -P 10 testdb
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 100
query mode: simple
number of clients: 10
number of threads: 1
number of transactions per client: 500
number of transactions actually processed: 5000/5000
latency average = 247.395 ms
latency stddev = 74.409 ms
initial connection time = 13.622 ms
tps = 39.713114 (without initial connection time)

PATCH:
./pgbench -c 10 -t 500 -P 10 testdb
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 100
query mode: simple
number of clients: 10
number of threads: 1
number of transactions per client: 500
number of transactions actually processed: 5000/5000
latency average = 251.757 ms
latency stddev = 72.846 ms
initial connection time = 13.025 ms
tps = 39.315862 (without initial connection time)

TEST SETUP:
primary in region 1
async standby 1 in the same region as that of the primary region 1
i.e. close to primary
sync standby 1 in region 2
sync standby 2 in region 3
an archive location in a region different from the primary and
standbys regions, region 4
Note that I intentionally kept sync standbys in regions far from
primary because it allows sync standbys to receive WAL a bit late by
default, which works well for our testing.

PGBENCH SETUP:
./psql -d postgres -c "drop database testdb"
./psql -d postgres -c "create database testdb"
./pgbench -i -s 100 testdb
./psql -d testdb -c "\dt"
./psql -d testdb -c "SELECT pg_size_pretty(pg_database_size('testdb'))"
./pgbench -c 10 -t 500 -P 10 testdb

Regards,
Bharath Rupireddy.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 25 Feb 2022 11:38:19 -0800

> report their flush lsn back to the primary. Please let me know your

I haven't had a chance to look too closely yet, but IIUC this adds a new
function that waits for synchronous replication.  This new function
essentially spins until the synchronous LSN has advanced.

I don't think it's a good idea to block sending any WAL like this.  AFAICT
it is possible that there will be a lot of synchronously replicated WAL
that we can send, and it might just be the last several bytes that cannot
yet be replicated to the asynchronous standbys.  І believe this patch will
cause the server to avoid sending _any_ WAL until the synchronous LSN
advances.

Perhaps we should instead just choose the SendRqstPtr based on the current
synchronous LSN.  Presumably there are other things we'd need to consider,
but in general, I think we ought to send as much WAL as possible for a
given call to XLogSendPhysical().

> the WAL, after all, that's the feature we are pursuing.

I'm curious what a longer pgbench run looks like when the synchronous
replicas are in the same region.  That is probably a more realistic
use-case.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: "Hsu, John" <[email-masked]>
Date: Fri, 25 Feb 2022 13:52:03 -0800

Hello,

On 2/25/22 11:38 AM, Nathan Bossart wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>

I think you're right that we'll avoid sending any WAL until sync_lsn 
advances. We could setup a contrived situation where the async-walsender 
never advances because it terminates before the flush_lsn of the 
synchronous_node catches up. And when the async-walsender restarts, 
it'll start with the latest flushed on the primary and we could go into 
a perpetual loop.

I took a look at the patch and tested basic streaming with async 
replicas ahead of the synchronous standby and with logical clients as 
well and it works as expected.

 >
 > ereport(LOG,
 >            (errmsg("async standby WAL sender with request LSN %X/%X 
is waiting as sync standbys are ahead with flush LSN %X/%X",
 >                    LSN_FORMAT_ARGS(flushLSN), 
LSN_FORMAT_ARGS(sendRqstPtr)),
 >             errhidestmt(true)));

I think this log formatting is incorrect.
s/sync standbys are ahead/sync standbys are behind/ and I think you need 
to swap flushLsn and sendRqstPtr

When a walsender is waiting for the lsn on the synchronous replica to 
advance and a database stop is issued to the writer, the pg_ctl stop 
isn't able to proceed and the database seems to never shutdown.

 > Assert(priority >= 0);

What's the point of the assert here?

Also the comments/code refer to AsyncStandbys, however it's also used 
for logical clients, which may or may not be standbys. Don't feel too 
strongly about the naming here but something to note.

 > if (!ShouldWaitForSyncRepl())
 >        return;
 > ...
 > for (;;)
 > {
 >    // rest of work
 > }

If we had a walsender already waiting for an ack, and the conditions of 
ShouldWaitForSyncRepl() change, such as disabling 
async_standbys_wait_for_sync_replication or synchronous replication 
it'll still wait since we never re-check the condition.

postgres=# select wait_event from pg_stat_activity where wait_event like 
'AsyncWal%';
               wait_event
--------------------------------------
  AsyncWalSenderWaitForSyncReplication
  AsyncWalSenderWaitForSyncReplication
  AsyncWalSenderWaitForSyncReplication
(3 rows)

postgres=# show synchronous_standby_names;
  synchronous_standby_names
---------------------------

(1 row)

postgres=# show async_standbys_wait_for_sync_replication;
  async_standbys_wait_for_sync_replication
------------------------------------------
  off
(1 row)

 >    LWLockAcquire(SyncRepLock, LW_SHARED);
 >    flushLSN = walsndctl->lsn[SYNC_REP_WAIT_FLUSH];
 >    LWLockRelease(SyncRepLock);

Should we configure this similar to the user's setting of 
synchronous_commit instead of just flush? (SYNC_REP_WAIT_WRITE, 
SYNC_REP_WAIT_APPLY)

Thanks,

John H