Re: More problems with VacuumPageHit style global variables

On Wed, Apr 20, 2022 at 8:00 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> I knew about pgBufferUsage, and I knew about
> VacuumPage{Hit,Miss,Dirty} for a long time. But somehow I didn't make
> the very obvious connection between the two until today. I am probably
> not the only one.

What about pgStatBlockWriteTime/pgstat_count_buffer_write_time(),
which also seem redundant? These were added by commit 64482890 back in
2012 (though under slightly different names), and are still used today
by code that aggregates database-level timing stats -- see
pgstat_update_dbstats().

Code like FlushBuffer() maintains both pgStatBlockWriteTime and
pgBufferUsage.blk_write_time (iff track_io_timing is on). So looking
at both the consumer side and the produce side makes it no more clear
why both are needed.

I suspect pgStatBlockWriteTime exists because of a similar kind of
historic confusion, or losing track of things. There are
similar-looking variables named things like pgStatXactCommit, which
are not redundant (since pgBufferUsage doesn't have any of that, just
granular I/O timing stuff). It would have been easy to miss the fact
that only a subset of these pgStat* variables were redundant. Also
seems possible that there was confusion about which variable was owned
by what subsystem, with the pgStat* stuff appearing to be a stats
collector thing, while pgBufferUsage appeared to be an executor thing.

I don't think that there is any risk of one user of either variable
"clobbering" some other user -- the current values of the variables
are not actually meaningful at all. They're only useful as a way that
an arbitrary piece of code instruments an arbitrary operation, by
making their own copies, running whatever the operation is, and then
reporting on the deltas. Which makes it even more surprising that this
was overlooked until now.

--
Peter Geoghegan