From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
cc: Thomas Munro <thomas.munro@gmail.com>,
        "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: suppressing useless wakeups in logical/worker.c
In-reply-to: <20230126003319.GA1507251@nathanxps13>
References: <20221202195503.GD2277157@nathanxps13>
 <TYAPR01MB58669549D0137CC76A9BB6A0F5189@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20221205173523.GA2714833@nathanxps13> <20230109174217.GA1112698@nathanxps13>
 <2557815.1674603908@sss.pgh.pa.us> <20230125235004.GA1327755@nathanxps13>
 <CA+hUKGJ=oFEX=TjeSCnGmqr_KseGLJxy=RnJMwRqPrX7N-Q2sg@mail.gmail.com>
 <20230126003319.GA1507251@nathanxps13>
Comments: In-reply-to Nathan Bossart <nathandbossart@gmail.com>
	message dated "Wed, 25 Jan 2023 16:33:19 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2878151.1674700077.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 25 Jan 2023 21:27:57 -0500
Message-ID: <2878152.1674700077@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2878152.1674700077%40sss.pgh.pa.us>
Precedence: bulk

Nathan Bossart <nathandbossart@gmail.com> writes:
> I think we might risk overflowing "long" when all the wakeup times are
> DT_NOEND:

> 	 * This is typically used to calculate a wait timeout for WaitLatch()
> 	 * or a related function.  The choice of "long" as the result type
> 	 * is to harmonize with that.  It is caller's responsibility that the
> 	 * input timestamps not be so far apart as to risk overflow of "long"
> 	 * (which'd happen at about 25 days on machines with 32-bit "long").

> Maybe we can adjust that function or create a new one to deal with this.

It'd probably be reasonable to file down that sharp edge by instead
specifying that TimestampDifferenceMilliseconds will clamp overflowing
differences to LONG_MAX.  Maybe there should be a clamp on the underflow
side too ... but should it be to LONG_MIN or to zero?

BTW, as long as we're discussing roundoff gotchas, I noticed while
testing your previous patch that there's some inconsistency between
TimestampDifferenceExceeds and TimestampDifferenceMilliseconds.
What you submitted at [1] did this:

+            if (TimestampDifferenceExceeds(last_start, now,
+                                           wal_retrieve_retry_interval))
+                ...
+            else
+            {
+                long        elapsed;
+
+                elapsed =3D TimestampDifferenceMilliseconds(last_start, n=
ow);
+                wait_time =3D Min(wait_time, wal_retrieve_retry_interval =
- elapsed);
+            }

and I discovered that that could sometimes busy-wait by repeatedly
falling through to the "else", but then calculating elapsed =3D=3D
wal_retrieve_retry_interval and hence setting wait_time to zero.
I fixed it in the committed version [2] by always computing "elapsed"
and then checking if that's strictly less than
wal_retrieve_retry_interval, but I bet there's existing code with the
same issue.  I think we need to take a closer look at making
TimestampDifferenceMilliseconds' roundoff behavior match the outcome of
TimestampDifferenceExceeds comparisons.

			regards, tom lane

[1] https://www.postgresql.org/message-id/20230110174345.GA1292607%40natha=
nxps13
[2] https://git.postgresql.org/gitweb/?p=3Dpostgresql.git&a=3Dcommitdiff&h=
=3D5a3a95385