Re: [PATCH v1 0/7] Wait event timing and tracing instrumentation

Hi,

On 2026-10-28 15:02:57 +0100, Dmitry Fomin wrote:
> Subject: [PATCH v13 1/5] Add begin/end hooks for timed wait events

I continue to be dead set against that. There's no way it's ok to just inject
arbitrary code into all the places that use wait events. Nor do I think it's
ok to increase the code size of all the places that use wait events.

> Add wait_event_begin_hook and wait_event_end_hook, called from a new
> pair of reporting functions, pgstat_report_wait_start_timed() and
> pgstat_report_wait_end_timed(), which an instrumented call site uses
> in place of the ordinary pair. The ordinary pgstat_report_wait_start()
> and pgstat_report_wait_end() are left exactly as they were, so an
> uninstrumented site, and any build with no extension loaded, behaves
> as it does today.

That's not a lot of consolation if your next patch converts everything to the
new way:

> Route the existing direct wait annotations in src/backend and src/common
> (94 starts and their 111 end and cleanup calls) through the explicitly
> instrumented pair, so that the hooks cover every wait site the backend
> executes today. The two src/common sites are the control file write and
> sync in update_controlfile(), inside the existing #ifndef FRONTEND
> guards; frontend builds of that file are unchanged.

I suggested earlier that it might be ok to convert *some* cases to timed wait
events, with one-by-one analysis why that's the best thing for that specific
wait event. I sure as hell didn't mean "all" by that.

I frankly just don't buy the premise that tracking the timing of wait events
alone is a useful exercise. Tracking the amount of time in specific wait
events is pretty much *useless* without other information. Knowing that
there's a lot of time in ClientRead doesn't tell you anything useful, without
also knowing whether it's in the middle of a statement or such. Even leaving
that extreme case aside, most of the other wait events aren't useful without
knowing how much time was spent doing CPU work.

Once you record with sufficient detail, you end up with the overhead being too
high to do this on every wait event.

I think it'd be much more valuable to have something that efficiently sampling
wait events at a configurable frequency, grouping it with information state,
backend_type, whether in a transaction, in a query.

Greetings,

Andres Freund