From: Tom Lane <tgl@sss.pgh.pa.us>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: suppressing useless wakeups in logical/worker.c
Date: Wed, 25 Jan 2023 21:27:57 -0500
Message-ID: <2878152.1674700077@sss.pgh.pa.us> (raw)
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>
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 = TimestampDifferenceMilliseconds(last_start, now);
+ wait_time = 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 ==
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%40nathanxps13
[2] https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=5a3a95385
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, nathandbossart@gmail.com, thomas.munro@gmail.com, kuroda.hayato@fujitsu.com
Subject: Re: suppressing useless wakeups in logical/worker.c
In-Reply-To: <2878152.1674700077@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