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

On Sat, Feb 26, 2022 at 3:22 AM Hsu, John <hsuchen(at)amazon(dot)com> wrote:
>
> > On Fri, Feb 25, 2022 at 08:31:37PM +0530, Bharath Rupireddy wrote:
> >> 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 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().
>
> 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.

The async walsender looks at flush LSN from
walsndctl->lsn[SYNC_REP_WAIT_FLUSH]; after it comes up and decides to
send the WAL up to it. If there are no sync replicats after it comes
up (users can make sync standbys async without postmaster restart
because synchronous_standby_names is effective with SIGHUP), then it
doesn't wait at all and continues to send WAL. I don't see any problem
with it. Am I missing something here?

> 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.

Thanks for reviewing and testing the patch.

> > 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

I will correct it. "async standby WAL sender with request LSN %X/%X is
waiting as sync standbys are ahead with flush LSN %X/%X",
LSN_FORMAT_ARGS(sendRqstP), LSN_FORMAT_ARGS(flushLSN). I will think
more about having better wording of these messages, any suggestions
here?

> 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.

I too observed this once or twice. It looks like the walsender isn't
detecting postmaster death in for (;;) with WalSndWait. Not sure if
this is expected or true with other wait-loops in walsender code. Any
more thoughts here?

> > Assert(priority >= 0);
>
> What's the point of the assert here?

Just for safety. I can remove it as the sync_standby_priority can
never be negative.

> 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.

I will try to be more informative by adding something like "async
standbys and logical replication subscribers".

> > 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.

Yeah, I will add the checks inside the async walsender wait-loop.

> 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)

As I said upthread, we can allow async standbys optionally wait for
either remote write or flush or apply or global min LSN of SendRqstPtr
so that users can choose what they want. I'm open to more thoughts
here.

Regards,
Bharath Rupireddy.