Re: [PATCH] Add recovery_min_apply_delay_reconnect recovery option

On Tue, Oct 17, 2017 at 12:51 AM, Eric Radman <ericshane(at)eradman(dot)com> wrote:
> This administrative compromise is necessary because the WalReceiver is
> not resumed after a network interruption until all records are read,
> verified, and applied from the archive on disk.

Taking a step back here... recoveryApplyDelay() uses
XLogCtl->recoveryWakeupLatch which gets set if the WAL receiver has
received new WAL, or if the WAL receiver shuts down properly. So if
the WAL receiver gets down for whatever reason during the loop of
recoveryApplyDelay(), the startup process waits for a record to be
applied maybe for a long time, and as there is no WAL receiver we
actually don't receive any new WAL records. New WAL records would be
received only once WaitForWALToBecomeAvailable() is called, which
happens once the apply delay is done for. If the postmaster dies, then
HandleStartupProcInterrupts() would take care of taking down
immediately the startup process, which is cool.

I see what you are trying to achieve and that seems worth it. It is
indeed a waste to not have a WAL receiver online while waiting for a
delay to be applied. If there is a flacky network between the primary
and a standby, you may end up with a standby way behind its primary,
and that could penalize a primary clean shutdown as the primary waits
for the shutdown checkpoint record to be flushed on the standby.

I think that your way to deal with the problem is messy though. If you
think about it, no parameters are actually needed. What you should try
to achieve is to make recoveryApplyDelay() smarter, by making the wait
to forcibly stop if you detect a failure by getting out of the redo
routine, and then force again the record to be read again. This way,
the startup process would try to start again a new WAL receiver if it
thinks that the source it should read WAL from is a stream. That may
turn to be a patch more complicated than you think though.

Your patch also breaks actually the use case of standbys doing
recovery using only archives and no streaming. In this case
WalRcvStreaming returns false, and recovery_min_apply_delay_reconnect
would be used unconditionally, so you would break a lot of
applications silently.

> Is it possible to verify the archive on disk independently of
> application? Adding a second delay parameter provides a workaround for
> some use cases without complecting xlog.c.

That's possible, not with core though. The archives are in a location
not controlled by the backend, but by archive_command, which may not
be local to the instance where Postgres is running. You could always
hack your own functions to do this work, here is an example of
something I came up with:
https://github.com/michaelpq/pg_plugins/tree/master/wal_utils
This prototype (use and hack at your own risk), is able to look at the
local contents of an archive. This can be used easily with a
client-side tool to copy a series of segments., or just perform sanity
checks on them.

For those reasons, -1 for the patch as proposed.
--
Michael