Re: Use instr_time for pg_stat_database block read/write time counters

Hi,

On 2026-10-02 07:57:38 +0900, Michael Paquier wrote:
> On Mon, Sep 28, 2026 at 02:24:52PM +0200, Bernd Reiß wrote:
> > @@ -125,7 +125,7 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> > {
> > if (io_op == IOOP_WRITE || io_op == IOOP_EXTEND)
> > {
> > - pgstat_count_buffer_write_time(INSTR_TIME_GET_MICROSEC(io_time));
> > + INSTR_TIME_ADD(pgStatBlockWriteTime, io_time);
> > if (io_object == IOOBJECT_RELATION)
> > INSTR_TIME_ADD(pgBufferUsage.shared_blk_write_time, io_time);
> > else if (io_object == IOOBJECT_TEMP_RELATION)
> > @@ -133,7 +133,7 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> > }
> > else if (io_op == IOOP_READ)
> > {
> > - pgstat_count_buffer_read_time(INSTR_TIME_GET_MICROSEC(io_time));
> > + INSTR_TIME_ADD(pgStatBlockReadTime, io_time);
> > if (io_object == IOOBJECT_RELATION)
> > INSTR_TIME_ADD(pgBufferUsage.shared_blk_read_time, io_time);
> > else if (io_object == IOOBJECT_TEMP_RELATION)
>
> Hmm. This part of the patch touches a performance-sensitive area.
> This is exchanging one addition for another, which I doubt really
> matters, but who knows.. Andres, any thoughts perhaps?

I think it should actually be a measurable win, due to getting rid of
INSTR_TIME_GET_MICROSEC(). That's a nontrivial conversion removed from the hot
path.

Doing the conversion from instr_time ticks once per stats flush, instead of
once per measurement, is a lot better.

Of course it'd be even better if would be to stop counting the same stuff in
pgBufferUsage.{shared,local}_blk_{read,write}_time and
pgStatBlock{Read,Write}Time. That's pretty darn silly.

pgstat_update_dbstats() should just keep a pgBufferUsage snapshot from the
last report and add up the relevant pgBufferUsage fields. And vacuum & analyze
already diff, so they just would need to add the fields to get the same
results.

Greetings,

Andres Freund