Re: 001_rep_changes.pl stalls

At Thu, 16 Apr 2020 22:41:46 -0700, Noah Misch <noah(at)leadboat(dot)com> wrote in
> On Mon, Apr 13, 2020 at 09:45:16PM -0400, Tom Lane wrote:
> > Noah Misch <noah(at)leadboat(dot)com> writes:
> > > This seems to have made the following race condition easier to hit:
> > > https://www.postgresql.org/message-id/flat/20200206074552.GB3326097%40rfd.leadboat.com
> > > https://www.postgresql.org/message-id/flat/21519.1585272409%40sss.pgh.pa.us
> >
> > Yeah, I just came to the same guess in the other thread.
> >
> > > While I don't think that indicates anything wrong with the fix for $SUBJECT,
> > > creating more buildfarm noise is itself bad. I am inclined to revert the fix
> > > after a week. Not immediately, in case it uncovers lower-probability bugs.
> > > I'd then re-commit it after one of those threads fixes the other bug. Would
> > > anyone like to argue for a revert earlier, later, or not at all?
> >
> > I don't think you should revert. Those failures are (just) often enough
> > to be annoying but I do not think that a proper fix is very far away.
>
> That works for me, but an actual defect may trigger a revert. Fujii Masao
> reported high walsender CPU usage after this patch. The patch caused idle
> physical walsenders to use 100% CPU. When caught up, the
> WalSndSendDataCallback for logical replication, XLogSendLogical(), sleeps
> until more WAL is available. XLogSendPhysical() just returns when caught up.
> No amount of WAL is too small for physical replication to dispatch, but
> logical replication needs the full xl_tot_len of a record. Some options:
>
> 1. Make XLogSendPhysical() more like XLogSendLogical(), calling
> WalSndWaitForWal() when no WAL is available. A quick version of this
> passes tests, but I'll need to audit WalSndWaitForWal() for things that are
> wrong for physical replication.
>
> 2. Make XLogSendLogical() more like XLogSendPhysical(), returning when
> insufficient WAL is available. This complicates the xlogreader.h API to
> pass back "wait for this XLogRecPtr", and we'd then persist enough state to
> resume decoding. This doesn't have any advantages to make up for those.
>
> 3. Don't avoid waiting in WalSndLoop(); instead, fix the stall by copying the
> WalSndKeepalive() call from WalSndWaitForWal() to WalSndLoop(). This risks
> further drift between the two wait sites; on the other hand, one could
> refactor later to help avoid that.
>
> 4. Keep the WalSndLoop() wait, but condition it on !logical. This is the
> minimal fix, but it crudely punches through the abstraction between
> WalSndLoop() and its WalSndSendDataCallback.
>
> I'm favoring (1). Other preferences?

Starting from the current shape, I think 1 is preferable, since that
waiting logic is no longer shared between logical and physical
replications. But I'm not sure I like calling WalSndWaitForWal()
(maybe with previous location + 1?), because the function seems to do
too-much.

By the way, if latch is consumed in WalSndLoop, succeeding call to
WalSndWaitForWal cannot be woke-up by the latch-set. Doesn't that
cause missing wakeups? (in other words, overlooking of wakeup latch).
Since the only source other than timeout of walsender wakeup is latch,
we should avoid wasteful consuming of latch. (It is the same issue
with [1]).

If wakeup signal is not remembered on walsender (like
InterruptPending), WalSndPhysical cannot enter a sleep with
confidence.

[1] https://www.postgresql.org/message-id/20200408.164605.1874250940847340108.horikyota.ntt@gmail.com

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center