Re: Disable startup progress timeout during standby WAL replay

> Maybe I'm missing your point... Could you clarify what change you are
> suggesting for EnableStandbyMode() itself?
>
> > > Wouldn't it be simpler to handle the standby case at the existing
> > > check, like this?
> > >
> > > if (!StandbyMode)
> > > begin_startup_progress_phase();
> > > + else
> > > + disable_startup_progress_timeout();
> >
> > I thought the same at first, too. But I just thought the timeout should not
> > be enabled even while reading the first WAL record, so I placed
> > the check before reading the first record.
>
> Similarly, if the timeout should be disabled before reading the first
> WAL record, I wonder whether begin_startup_progress_phase() should
> also be called at the same point.

I think disable_startup_progress_timeout() is needed both in
EnableStandbyMode() and near the beginning of PerformWalRecovery(),
because the two calls cover different orderings. When standby mode is
enabled during InitWalRecovery(), it happens before
ResetUnloggedRelations(). Since ResetUnloggedRelations() calls
begin_startup_progress_phase(), it can re-enable the timeout after
EnableStandbyMode() has disabled it. Therefore, if StandbyMode is
already true on entry to PerformWalRecovery(), the timeout needs to be
disabled again before reading the first WAL record.

On the other hand, recovery can initially start with StandbyMode ==
false and later switch from crash recovery to archive/standby recovery
after exhausting the WAL available in pg_wal. In that case,
EnableStandbyMode() is called from ReadRecord(), and a WAL replay
progress timeout may already be active. Therefore, the call in
EnableStandbyMode() also needs to remain. This transition can
potentially happen while trying to obtain the first replay record,
before InRedo becomes true. Thus, making the call in
EnableStandbyMode() conditional on replay having already started would
not cover every case.

Regarding the location of the new call, placing it before the first
ReadRecord() looks correct to me. Reading the first record may involve
restoring WAL from the archive or waiting for WAL to arrive. If the
timeout was left active by ResetUnloggedRelations(), postponing the
disable until after ReadRecord() would allow unnecessary periodic
timeout activity throughout that wait.

I do not think begin_startup_progress_phase() should be moved to the
same location for symmetry. Its current placement after successfully
obtaining the first record, near the "redo starts at ..." message,
provides a natural boundary for the reported redo phase. Moving it
before ReadRecord() would include time spent locating, restoring, or
waiting for the first WAL record in the reported redo elapsed time.
There is also no redo progress-reporting point inside that initial
ReadRecord() call.

Therefore, although the two disable calls look similar, they handle
different transitions, and the code change in the patch looks correct
to me.

I have one minor suggestion for the documentation. The proposed wording:

> On a standby server, however, it does not apply during WAL replay.

might be slightly broader than the actual condition, because a server
requested to become a standby can initially perform crash recovery
with StandbyMode == false. Perhaps the following would map more
directly to the implementation:

"This setting is applied separately to each operation, such as WAL
replay, syncing the data directory, and resetting unlogged relations.
WAL replay progress is not reported while standby mode is active."

With that minor documentation suggestion, the patch looks good to me.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft