On Mon, Feb 13, 2023 at 11:31:03AM +0530, Bharath Rupireddy wrote:
> Needed a rebase. Please review the attached v8 patch set.
I was looking at this patch, and got a few comments.
FWIW, I kind of agree with the feeling of Bertrand upthread that using
"checkpoint_" in the attribute names for the new view is globally
inconsistent. After 0003, we get:
=# select attname from pg_attribute
where attrelid = 'pg_stat_checkpointer'::regclass
and attnum > 0;
attname
-----------------------------
timed_checkpoints
requested_checkpoints
checkpoint_write_time
checkpoint_sync_time
buffers_written_checkpoints
stats_reset
(6 rows)
=# select attname from pg_attribute
where attrelid = 'pg_stat_bgwriter'::regclass and
attnum > 0;
attname
------------------
buffers_clean
maxwritten_clean
buffers_alloc
stats_reset
(4 rows)
The view for the bgwriter does not do that. I'd suggest to use
functions that are named as pg_stat_get_checkpoint_$att with shorter
$atts. It is true that "timed" is a bit confusing, because it refers
to a number of checkpoints, and that can be read as a time value (?).
So how about num_timed? And for the others num_requested and
buffers_written?
+ * Unlike the checkpoint fields, reqquests related fields are protected by
s/reqquests/requests/.
SlruSyncFileTag(SlruCtl ctl, const FileTag *ftag, char *path)
{
+ SlruShared shared = ctl->shared;
int fd;
int save_errno;
int result;
+ /* update the stats counter of flushes */
+ pgstat_count_slru_flush(shared->slru_stats_idx);
Why is that in 0002? Isn't that something we should treat as a bug
fix of its own, even backpatching it to make sure that the flush
requests for individual commit_ts, multixact and clog files are
counted in the stats?
Saying that, I am OK with moving ahead with 0001 and 0002 to remove
buffers_backend_fsync and buffers_backend from pg_stat_bgwriter, but
it is better to merge them into a single commit. It helps with 0003
and this would encourage the use of pg_stat_io that does a better job
overall with more field granularity.
--
Michael