| From: | Lukas Fittl <lukas(at)fittl(dot)com> |
|---|---|
| To: | Andres Freund <andres(at)anarazel(dot)de> |
| Cc: | ahmed <gouda0x(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Melanie Plageman <melanieplageman(at)gmail(dot)com>, Bernd Reiß <bd_reiss(at)gmx(dot)at>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Use instr_time for pg_stat_database block read/write time counters |
| Date: | 2026-10-16 17:53:41 |
| Message-ID: | CAP53Pkz-o8goa6jR_bP_+jJtLHy3hf=OHBqCid=3ttjrT+oLjA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Oct 2, 2026 at 11:40 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > We tried to implement this and directly use `pgBufferUsage` in
> > `pgstat_update_dbstats()` but we came across some issues that caused
> > pg_stat_database to overcount, specifically when having parallel workers in
> > the plan, each worker will acuumulate its own statistics in its own
> > `pgBufferUsage` and will report them at the end and update the db stats
> > entry, but the problem is after that the parent backend will also acuumulate
> > all the statistics from its parallel workers and add them to its
> > `pgBufferUsage` and update the db stats entry again hence causing each
> > worker's I/O write/read times to be counted twice.
>
> I really really dislike a lot of how the parallel worker stats stuff is done
> :(. The whole idea of just mixing it into the client backends stats is just
> bonkers IMO. We do it inconsistently, sometimes we then have to back out the
> worker numbers again during explain. Just a bad idea overall.
Mhm. The way that parallel worker instrumentation is done is very
complex and brittle. It's easy to get any changes there wrong,
introducing double counting/etc.
> I think Lukas had some WIP patches to change some of that? He might also have
> some opinions independent of parallel workers, as he has pending work to
> refactor things so that the explain stuff is not diff based anymore (the
> diffing makes it rather expensive atm).
>
> So maybe this is all moot - I'll let him chime in on that.
Yeah, the stack-based instrumentation patch works some related code,
but notably I didn't touch that call to pgStatBlock(Write|Read)Time in
analyze.c/vacuumlazy.c on prior iterations.
Looking at it again, if I read this right, if we were to sum up local
and shared I/O timings (pgstat_count_io_op_time already ignores
IOOBJECT_WAL for pgstat_count_buffer_...), we should indeed get the
same measurements via the instrumentation buffer usage data, avoiding
the need to do separate diffing. I have a draft of that locally and
hoping to share this in the next few days on the existing thread for
stack-based instrumentation.
Whilst looking at this last week I also noticed a related bug with
parallel VACUUM not reporting I/O times correctly, but coincidentally
that was fixed with the independently discussed
abf5fec634f6aac98e80e2ef6c3f3dbf0133dfaa - so the remaining user of
pgStatBlock(Read|Write)Time is only ANALYZE and pg_stat_database, both
of which could switch to the stack-based approach.
Thanks,
Lukas
--
Lukas Fittl
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Lucas Jeffrey | 2026-10-16 17:56:15 | Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) |
| Previous Message | Masahiko Sawada | 2026-10-16 17:45:43 | Re: Parallel vacuum: workers together with leader could overrun cost budget |