On Thu, Aug 5, 2021 at 7:41 AM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
> This seems a little confusing. As we are setting
> last_startup_progress_timeout = now and then calling
> reset_startup_progress_timeout() which will calculate the next_time
> based on the value of last_startup_progress_timeout initially and
> checks whether next_timeout is less than now. It doesn't make sense to
> me. I feel we should calculate the next_timeout based on the time that
> it is supposed to fire next time. So we should set
> last_startup_progress_timeout = next_timeout after enabling the timer.
> Also I feel with the 2 functions mentioned above, we also need
> InitStartupProgress() which sets the initial value to
> startupProcessOpStartTime which is used to calculate the time
> difference and display in the logs. I could see those functions like
> below.
>
> InitStartupProgress(void)
> {
> startupProcessOpStartTime = GetCurrentTimestamp();
> ResetStartupProgressTimeout(startupProcessOpStartTime);
> }
This makes sense, but I think I'd like to have all the functions in
this patch use names_like_this() rather than NamesLikeThis().
> reset_startup_progress_timeout(TimeStampTz now)
> {
> next_timeout = last_startup_progress_timeout + interval;
> if (next_timeout < now)
> {
> // Either the timeout was processed so late that we missed an entire cycle,
> // or the system clock was set backwards.
> next_timeout = now + interval;
> }
> enable_timeout_at(next_timeout);
> last_startup_progress_timeout = next_timeout;
> }
Hmm, yeah, that seems good, but given this change, maybe the variables
need a little renaming. Like change last_startup_progress_timeout to
scheduled_startup_progress_timeout, perhaps.
> startup_progress_timeout_has_expired()
> {
> if (!startup_progress_timer_expired)
> return;
> now = GetCurrentTimestamp();
> // compute timestamp difference based on startupProcessOpStartTime
> reset_startup_progress_timeout(now);
> }
I guess this one needs to return a Boolean, actually.
--
Robert Haas
EDB: http://www.enterprisedb.com
