| From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | Michael Paquier <michael(at)paquier(dot)xyz> |
| Cc: | Tatsuya Kawata <kawatatatsuya0913(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Change wait_time column of pg_stat_lock to double precision |
| Date: | 2026-06-29 04:27:55 |
| Message-ID: | akH0SxXlXPNjD+R5@bdtpg |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Smith | 2026-06-29 04:30:22 | Re: DOCS - "Get Object DDL Functions" table improvements |
| Previous Message | Amit Kapila | 2026-06-29 04:27:54 | Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server |