Re: walsender vs. XLogBackgroundFlush during shutdown

Hi,

On 2019-05-01 02:28:45 +0200, Tomas Vondra wrote:
> The reason for that is pretty simple - the walsenders are doing logical
> decoding, and during shutdown they're waiting for WalSndCaughtUp=true.
> But per XLogSendLogical() this only happens if
>
> if (logical_decoding_ctx->reader->EndRecPtr >= GetFlushRecPtr())
> {
> WalSndCaughtUp = true;
> ...
> }
>
> That is, we need to get beyong GetFlushRecPtr(). But that may never
> happen, because there may be incomplete (and unflushed) page in WAL
> buffers, not forced out by any transaction. So if there's a WAL record
> overflowing to the unflushed page, the walsender will never catch up.
>
> Now, this situation is apparently expected, because WalSndWaitForWal()
> does this:
>
> /*
> * If we're shutting down, trigger pending WAL to be written out,
> * otherwise we'd possibly end up waiting for WAL that never gets
> * written, because walwriter has shut down already.
> */
> if (got_STOPPING)
> XLogBackgroundFlush();
>
> but unfortunately that does not actually do anything, because right at
> the very beginning XLogBackgroundFlush() does this:
>
> /* back off to last completed page boundary */
> WriteRqst.Write -= WriteRqst.Write % XLOG_BLCKSZ;

> That is, it intentionally ignores the incomplete page, which means the
> walsender can't read the record and reach GetFlushRecPtr().
>
> XLogBackgroundFlush() does this since (at least) 2007, so how come we
> never had issues with this before?

I assume that's because of the following logic:
/* if we have already flushed that far, consider async commit records */
if (WriteRqst.Write <= LogwrtResult.Flush)
{
SpinLockAcquire(&XLogCtl->info_lck);
WriteRqst.Write = XLogCtl->asyncXactLSN;
SpinLockRelease(&XLogCtl->info_lck);
flexible = false; /* ensure it all gets written */
}

and various pieces of the code doing XLogSetAsyncXactLSN() to force
flushing. I wonder if the issue is that we're better at avoiding
unnecessary WAL to be written due to
6ef2eba3f57f17960b7cd4958e18aa79e357de2f

> But I don't think we're safe without the failover slots patch, because
> any output plugin can do something similar - say, LogLogicalMessage() or
> something like that. I'm not aware of a plugin doing such things, but I
> don't think it's illegal / prohibited either. (Of course, plugins that
> generate WAL won't be useful for decoding on standby in the future.)

FWIW, I'd consider such an output plugin outright broken.

> So what I think we should do is to tweak XLogBackgroundFlush() so that
> during shutdown it skips the back-off to page boundary, and flushes even
> the last piece of WAL. There are only two callers anyway, so something
> like XLogBackgroundFlush(bool) would be simple enough.

I think it then just ought to be a normal XLogFlush(). I.e. something
along the lines of:

/*
* If we're shutting down, trigger pending WAL to be written out,
* otherwise we'd possibly end up waiting for WAL that never gets
* written, because walwriter has shut down already.
*/
if (got_STOPPING && !RecoveryInProgress())
XLogFlush(GetXLogInsertRecPtr());

/* Update our idea of the currently flushed position. */
if (!RecoveryInProgress())
RecentFlushPtr = GetFlushRecPtr();
else
RecentFlushPtr = GetXLogReplayRecPtr(NULL);

Greetings,

Andres Freund