Re: pg_usleep for multisecond delays

Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
> On Fri, Feb 10, 2023 at 10:51:20AM -0800, Nathan Bossart wrote:
>> On Fri, Feb 10, 2023 at 10:18:34AM -0500, Tom Lane wrote:
>>> BTW, we have an existing pg_sleep() function that deals with all
>>> of this except re-setting the latch.

> Here is a work-in-progress patch. I quickly scanned through my previous
> patch and applied this new function everywhere it seemed safe to use (which
> unfortunately is rather limited).

A quick grep for pg_usleep with large intervals finds rather more
than you touched:

contrib/auth_delay/auth_delay.c: 46: pg_usleep(1000L * auth_delay_milliseconds);
src/backend/access/nbtree/nbtpage.c: 2979: pg_usleep(5000000L);
src/backend/access/transam/xlog.c: 5109: pg_usleep(60000000L);
src/backend/libpq/pqcomm.c: 717: pg_usleep(100000L); /* wait 0.1 sec */
src/backend/postmaster/bgwriter.c: 199: pg_usleep(1000000L);
src/backend/postmaster/checkpointer.c: 313: pg_usleep(1000000L);
src/backend/postmaster/checkpointer.c: 1009: pg_usleep(100000L); /* wait 0.1 sec, then retry */
src/backend/postmaster/postmaster.c: 4295: pg_usleep(PreAuthDelay * 1000000L);
src/backend/postmaster/walwriter.c: 192: pg_usleep(1000000L);
src/backend/postmaster/bgworker.c: 762: pg_usleep(PostAuthDelay * 1000000L);
src/backend/postmaster/pgarch.c: 456: pg_usleep(1000000L);
src/backend/postmaster/pgarch.c: 488: pg_usleep(1000000L); /* wait a bit before retrying */
src/backend/postmaster/autovacuum.c: 444: pg_usleep(PostAuthDelay * 1000000L);
src/backend/postmaster/autovacuum.c: 564: pg_usleep(1000000L);
src/backend/postmaster/autovacuum.c: 690: pg_usleep(1000000L); /* 1s */
src/backend/postmaster/autovacuum.c: 1711: pg_usleep(PostAuthDelay * 1000000L);
src/backend/storage/ipc/procarray.c: 3799: pg_usleep(100 * 1000L); /* 100ms */
src/backend/utils/init/postinit.c: 985: pg_usleep(PostAuthDelay * 1000000L);
src/backend/utils/init/postinit.c: 1192: pg_usleep(PostAuthDelay * 1000000L);

Did you have reasons for excluding the rest of these?

Taking a step back, I think it might be a mistake to try to share code
with the SQL-exposed function; at least, that is causing some API
decisions that feel odd. I have mixed emotions about both the use
of double as the sleep-time datatype, and about the choice of seconds
(rather than, say, msec) as the unit. Admittedly this is not all bad
--- for example, several of the calls I list above are delaying for
100ms, which we can easily accommodate in this scheme as "0.1", and
maybe it'd be a good idea to hit up the stuff waiting for 10ms too.
It still seems unusual for an internal support function though.
I haven't done the math on it, but are we limiting the precision
of the sleep (due to roundoff error) in any way that would matter?

A bigger issue is that we almost certainly ought to pass through
a wait event code instead of allowing all these cases to be
WAIT_EVENT_PG_SLEEP.

I'd skip the unconditional latch reset you added to pg_sleep_sql.
I realize that's bug-compatible with what happens now, but I still
think it's expending extra code to do what might well be the
wrong thing.

We should update the header comment for pg_usleep to direct people
to this new function.

regards, tom lane