Re: Reporting WAL replay progress during pre-consistency standby reovery

Hi Nitin,

I've been looking at this patch and had some thoughts.

Regarding Fujii's point about the process title, it's useful while someone
is watching, but it only shows the segment, has to be polled,
and leaves nothing behind afterwards. Before consistency the standby
can't be queried, so the server log is the only lasting record. One more
case where this matters, which is after
pg_rewind where backup_label points to the last common checkpoint before
the divergence, and minRecoveryPoint is set to the source's insert LSN
at the end of the rewind. Everything in between is replayed before
consistency, mostly from local pg_wal, so pg_stat_replication doesn't
cover it either. The patch stops reporting at consistency, so the
log-volume reason for suppressing these messages on a standby doesn't
apply.

v1 applies cleanly to current master and builds without warnings.

I think this is worth having and have a few comments:

1. In EnableStandbyMode():

if (!InRedo || reachedConsistency)
disable_startup_progress_timeout();

As far as I can see, the only caller that runs during redo is the
crash-to-archive recovery switch in ReadRecord(). That code then sets
minRecoveryPoint to xlogreader->EndRecPtr and calls
CheckRecoveryConsistency(). backupEndRequired can't be set on that
path (otherwise we'd have entered archive recovery directly), so
consistency is reached immediately, and the new
disable_startup_progress_timeout() in CheckRecoveryConsistency() turns
the timer off anyway. Unless I'm missing a path, this code could be
dropped, keeping the original unconditional disable.

2. disable_startup_progress_timeout() returns early when
log_startup_progress_interval is 0:

/* Feature is disabled. */
if (log_startup_progress_interval == 0)
return;

The setting is PGC_SIGHUP and the startup process rereads it during replay.
I tested this on a standby that took about 20s to reach
consistency, with log_startup_progress_interval = 1s, reloading it to 0 a
few seconds into replay. After "consistent recovery state reached"
the timer kept firing every second until shutdown (I counted the fires with
a temporary counter in the handler), because the disable call in
CheckRecoveryConsistency() does nothing. On master the redo timer is
only enabled outside standby mode, where recovery normally ends soon. With
this patch the timer can stay enabled on a normal long-running
standby. Removing the early return fixes it in my test (assert-enabled
build). Every caller runs after StartupXLOG() has registered
STARTUP_PROGRESS_TIMEOUT, so disable_timeout() is safe there.

Relatedly, after the reload to 0 the "redo in progress" messages also kept
appearing until consistency, since
has_startup_progress_timeout_expired() doesn't check the setting.
Crash recovery uses the same code, so that may be out of scope here.

The documentation change looks fine.

Regards,
Surya Poondla