From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: Misuse of TimestampDifference() in the autoprewarm feature of pg_prewarm
Date: Mon, 09 Nov 2020 13:53:25 -0500
Message-ID: <1992985.1604948005@sss.pgh.pa.us> (raw)
In-Reply-To: <3b1c053a21c07c1ed5e00be3b2b855ef@postgrespro.ru>
References: <3b1c053a21c07c1ed5e00be3b2b855ef@postgrespro.ru>
Alexey Kondratov <a.kondratov@postgrespro.ru> writes:
> After fixing this issue I have noticed that it still dumps blocks twice
> at each timeout (here I set autoprewarm_interval to 15s):
> ...
> This happens because at timeout time we were using continue, but
> actually we still have to wait the entire autoprewarm_interval after
> successful dumping.
I don't think your 0001 is correct. It would be okay if apw_dump_now()
could be counted on to take negligible time, but we shouldn't assume
that should we?
I agree that the "continue" seems a bit bogus, because it's skipping
the ResetLatch call at the bottom of the loop; it's not quite clear
to me whether that's a good thing or not. But the general idea of
the existing code seems to be to loop around and make a fresh calculation
of how-long-to-wait, and that doesn't seem wrong.
0002 seems like a pretty clear bug fix, though I wonder if this is exactly
what we want to do going forward. It seems like a very large fraction of
the callers of TimestampDifference would like to have the value in msec,
which means we're doing a whole lot of expensive and error-prone
arithmetic to break down the difference to sec/usec and then put it
back together again. Let's get rid of that by inventing, say
TimestampDifferenceMilliseconds(...).
BTW, I see another bug of a related ilk. Look what
postgres_fdw/connection.c is doing:
TimestampDifference(now, endtime, &secs, µsecs);
/* To protect against clock skew, limit sleep to one minute. */
cur_timeout = Min(60000, secs * USECS_PER_SEC + microsecs);
/* Sleep until there's something to do */
wc = WaitLatchOrSocket(MyLatch,
WL_LATCH_SET | WL_SOCKET_READABLE |
WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
PQsocket(conn),
cur_timeout, PG_WAIT_EXTENSION);
WaitLatchOrSocket's timeout is measured in msec not usec. I think the
comment about "clock skew" is complete BS, and the Min() calculation was
put in as a workaround by somebody observing that the sleep waited too
long, but not understanding why.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, a.kondratov@postgrespro.ru, pgsql-hackers@lists.postgresql.org, robertmhaas@gmail.com
Subject: Re: Misuse of TimestampDifference() in the autoprewarm feature of pg_prewarm
In-Reply-To: <1992985.1604948005@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox