From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
        Robert Haas <robertmhaas@gmail.com>
Subject: Re: Misuse of TimestampDifference() in the autoprewarm feature of
 pg_prewarm
In-reply-to: <3b1c053a21c07c1ed5e00be3b2b855ef@postgrespro.ru>
References: <3b1c053a21c07c1ed5e00be3b2b855ef@postgrespro.ru>
Comments: In-reply-to Alexey Kondratov <a.kondratov@postgrespro.ru>
	message dated "Mon, 09 Nov 2020 19:45:18 +0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1992984.1604948005.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 09 Nov 2020 13:53:25 -0500
Message-ID: <1992985.1604948005@sss.pgh.pa.us>
Precedence: bulk

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, &microsecs);

                /* To protect against clock skew, limit sleep to one minut=
e. */
                cur_timeout =3D Min(60000, secs * USECS_PER_SEC + microsec=
s);

                /* Sleep until there's something to do */
                wc =3D 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