Thread: enable_timeout_every() and fin_time

enable_timeout_every() and fin_time

From
Andres Freund
Date:
Hi,

I was looking using enable_timeout_every() in another place with Lukas
just now, and noticed the fin_time argument.  It seems odd for an
interval firing interface to get an absolute timestamp as an
argument. The only in-tree user of enable_timeout_every() computes
fin_time explicitly using the interval time:

    startup_progress_phase_start_time = GetCurrentTimestamp();
    fin_time = TimestampTzPlusMilliseconds(startup_progress_phase_start_time,
                                           log_startup_progress_interval);
    enable_timeout_every(STARTUP_PROGRESS_TIMEOUT, fin_time,
                         log_startup_progress_interval);

In https://postgr.es/m/CA%2BTgmoYqSF5sCNrgTom9r3Nh%3Dat4WmYFD%3DgsV-omStZ60S0ZUQ%40mail.gmail.com
Robert said:
> Apparently not, but here's a v2 anyway. In this version I made
> enable_timeout_every() a three-argument function, so that the caller
> can specify both the first time at which the timeout routine should be
> called and the interval between them, instead of only the latter. That
> seems to be more convenient for this use case, and is more powerful in
> general.

What is the use case for an absolute start time plus a relative
interval?

ISTM that this will just lead to every caller ending up with a
calculation like the startup.c piece quoted above.

Greetings,

Andres Freund



Re: enable_timeout_every() and fin_time

From
Robert Haas
Date:
On Sun, Jan 1, 2023 at 7:36 PM Andres Freund <andres@anarazel.de> wrote:
> What is the use case for an absolute start time plus a relative
> interval?

The code snippet that you indicate has the important side effect of
changing the global variable startup_progress_phase_start_time, which
is used by has_startup_progress_timeout_expired. Without the fin_time
argument, the timeout machinery would have to call
GetCurrentTimestamp() separately, and the caller wouldn't know what
answer it got. The result would be that the progress reports would
indicate an elapsed time relative to one timestamp, but the time at
which those progress reports were printed would be relative to a
slightly different timestamp.

Maybe nobody would notice such a minor discrepancy, but I wanted to avoid it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com



Re: enable_timeout_every() and fin_time

From
Andres Freund
Date:
Hi,

On 2023-01-03 13:33:34 -0500, Robert Haas wrote:
> On Sun, Jan 1, 2023 at 7:36 PM Andres Freund <andres@anarazel.de> wrote:
> > What is the use case for an absolute start time plus a relative
> > interval?
> 
> The code snippet that you indicate has the important side effect of
> changing the global variable startup_progress_phase_start_time, which
> is used by has_startup_progress_timeout_expired. Without the fin_time
> argument, the timeout machinery would have to call
> GetCurrentTimestamp() separately, and the caller wouldn't know what
> answer it got. The result would be that the progress reports would
> indicate an elapsed time relative to one timestamp, but the time at
> which those progress reports were printed would be relative to a
> slightly different timestamp.

> Maybe nobody would notice such a minor discrepancy, but I wanted to avoid it.

Doesn't that discrepancy already exist as the code stands, because
startup_progress_phase_start_time is also set in
has_startup_progress_timeout_expired()? I realize that was an example, but the
issue seems broader: After the first "firing", the next timeout will be
computed relative to an absolute time gathered in timestamp.c.

Greetings,

Andres Freund



Re: enable_timeout_every() and fin_time

From
Robert Haas
Date:
On Tue, Jan 3, 2023 at 3:14 PM Andres Freund <andres@anarazel.de> wrote:
> Doesn't that discrepancy already exist as the code stands, because
> startup_progress_phase_start_time is also set in
> has_startup_progress_timeout_expired()?

I don't think it is, actually.

> I realize that was an example, but the
> issue seems broader: After the first "firing", the next timeout will be
> computed relative to an absolute time gathered in timestamp.c.

We're computing the time since the start of the current phase, not the
time since the last timeout. So I don't see how this is relevant.

-- 
Robert Haas
EDB: http://www.enterprisedb.com