Re: Disable startup progress timeout during standby WAL replay

> On Oct 15, 2026, at 16:26, Aidar Imamov <imamovaj22(at)gmail(dot)com> wrote:
>
>> On Oct 7, 2026, at 12:54, Nitin Jadhav <nitinjadhavpostgres(at)gmail(dot)com> wrote:
>>
>>> 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
>>
>>
>
> Hi!
>
> Thanks for the patch. The problem is real, and I agree with Fujii's
> analysis and Nitin's: the fix is correct, the placement is right, and
> the call in EnableStandbyMode() still needs to stay.
>
> I built master with and without the patch, with a temporary write() in
> startup_progress_timeout_handler() to count firings, and
> the timer re-armed by ResetUnloggedRelations() did keep firing for
> the standby's whole lifetime, and the patch stops it.
>
> I also checked the crash-recovery-first path (a crashed primary
> restarted with standby.signal, no backup_label). There redo progress is
> reported during the crash recovery part, and the timer is still running
> when ReadRecord() calls EnableStandbyMode(). So the existing call in
> EnableStandbyMode() is still needed; the new call only covers the case
> where StandbyMode was already true when PerformWalRecovery() started.
>
> Two non-blocking suggestions, no new patch needed for them:
>
> - The comment says "earlier startup phases executed afterward", which
> contradicts itself, and EnableStandbyMode() disables the timer, not
> the logging (the !StandbyMode checks do that). Maybe:
>
> EnableStandbyMode() disabled the startup progress timeout, but
> startup operations performed since then, such as
> ResetUnloggedRelations(), may have re-enabled it. Disable it again,
> since recovery progress is not reported in standby mode.
>
> - The doc sentence "On a standby server, however, it does not apply
> during WAL replay" (and Nitin's "while standby mode is active") is
> slightly off for the standby.signal-without-backup_label case: the
> server logs "entering standby mode" and then still emits "redo in
> progress" during the crash recovery part, because StandbyMode only
> becomes true when ReadRecord() switches over. The exception only ever
> adds messages and never hides any, so maybe a simple sentence is
> safest, like:
> "Progress of WAL replay is not reported on a standby
> server."
>
> Separately, while testing the conflict path I found a real bug next to
> this one, and I'm attaching a small patch for it.
>
> ResolveRecoveryConflictWithLock() and
> ResolveRecoveryConflictWithBufferPin() both end with
> disable_all_timeouts(false), under the assumption that the startup
> process has no other timeouts. That stopped being true in PG 15, when
> STARTUP_PROGRESS_TIMEOUT was added. So a single recovery conflict
> cancels the progress timeout, and "redo in progress" goes quiet for the
> rest of the redo, even though redo keeps going.
>
> I reproduced it with PITR (recovery.signal, hot_standby=on): without a conflict
> I got 24 messages of redo; with one lock conflict 7 messages, then silence
> only. Nothing is lost but the log messages, so an admin
> watching recovery might well think it is stuck. The attached patch
> cancels only the two timeouts those functions enable; with it the same
> run gives 24 messages.
>
> That one is a separate and pre-existing: probably wants a backpatch
> to 15. I exercised the lock-conflict path; the buffer-pin path is the
> same pattern but I only read it, not ran it.
>
> Regards,
> Aidar Imamov
>
> <0001-Fix-recovery-conflict-canceling-the-startup-progress.patch>

A small clarification about the v2 I just attached.

It merges your v1 with my extra hunk into a single patch. The reason is
purely the commitfest bot: if I upload two patches, it tends to show
only the latest one and the original patch can get lost in the
pipeline. Keeping everything in one patch lets the bot and reviewers see
the whole picture in one place, and my hunk does not overlap or hide
yours.

That said, I am not sure this is the right shape. The extra hunk fixes
a separate and pre-existing thing, and it probably wants its own thread and
a backpatch to 15 if it worths. So bundling it into this thread may be the wrong
move. I did it only so the bot does not lose track of things.

If you or anyone else thinks the two should live apart, just say so and
I will split them: your v1 stays as is, and I will move the
disable_all_timeouts() fix to a new thread.

Regards,
Aidar Imamov