Hi,
On Mon, Jun 29, 2026 at 11:11:06AM +0900, Michael Paquier wrote:
> On Sun, Jun 28, 2026 at 02:50:38AM +0900, Tatsuya Kawata wrote:
> > Thank you for your review!
>
> >> -pgstat_count_lock_waits(uint8 locktag_type, long msecs)
> >> +pgstat_count_lock_waits(uint8 locktag_type, double msecs)
> >>
> >> What about keeping the long and rename to usecs?
> >
> > Agreed. Fixed.
>
> Relying on long here is not reliable on WIN32, where the value could
> overflow for long wait times (where sizeof(long) == 4). I'd suggest a
> int64 or a uint64 instead for the API to be able to store wait times
> in usecs longer than the overflow threshold. That was another design
> oversight. My oversight here (aka I want to abolish completely the
> use of long in the core code; it leads to insanity).
That makes sense and the lock time that would produce the overflow is realistic
(about 35 minutes) if my math is correct.
> >> values[i++] = Float8GetDatum((double) lck_stats->wait_time / 1000.0);
> >>
> >> instead?
> >
> > Agreed. Fixed.
>
> We have a pg_stat_us_to_ms() that can do this job as well for the
> values displayed. Why duplicate this code? This conversion routine
> is used by pg_stat_io.
Right. That said I just observed that it can produces things like:
postgres=# select read_time from pg_stat_io where read_time > 0;
read_time
---------------------
2.2640000000000002
0.08700000000000001
Maybe it should be changed to?
return (double) val_ms / 1000.0;
instead of:
return val_ms * (double) 0.001;
> >>
> >> The idea being to keep the PgStat_Counter type, the long parameter type
> > and
> >> do the conversion at display time.
> >
> > Agreed. Fixed.
>
> - PgStat_Counter wait_time; /* time in milliseconds */
> + PgStat_Counter wait_time; /* time in microseconds */
>
> The counter is stored in usecs, displayed in msecs. Documenting it as
> stored in msecs is incorrect, no?
Yeah, so this change.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
