Re: Introduce a new view for checkpointer related stats

On Thu, Oct 26, 2023 at 10:55:00PM +0530, Bharath Rupireddy wrote:
> On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> 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?
>
> +1. I included the fix in a separate patch 0002 here.

Hmm. As per the existing call of pgstat_count_slru_flush() in
SimpleLruWriteAll(), routine called SimpleLruFlush() until ~13 and
dee663f78439, an incrementation of 1 for slru_stats_idx refers to all
the flushes for all the dirty data of this SLRU in a single pass.
This addition actually means that we would now increment the counter
for each sync request, changing its meaning. Perhaps there is an
argument for changing the meaning of this parameter to be based on the
number of flush requests completed, but if that were to happen it
would be better to remove pgstat_count_slru_flush() in
SimpleLruWriteAll(), I guess, or just aggregate this counter once,
which would be cheaper.

>> 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.
>
> I merged v8 0001 and 0002 into one single patch, PSA v9-0001.

v9-0001 is OK, so I have applied it.

v9-0003 is mostly a mechanical change, and it passes the eye test. I
have spotted two nits.

+CREATE VIEW pg_stat_checkpointer AS
+ SELECT
+ pg_stat_get_checkpointer_num_timed() AS num_timed,
+ pg_stat_get_checkpointer_num_requested() AS num_requested,
+ pg_stat_get_checkpointer_write_time() AS write_time,
+ pg_stat_get_checkpointer_sync_time() AS sync_time,
+ pg_stat_get_checkpointer_buffers_written() AS buffers_written,
+ pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;

Okay with this layer. I am wondering if others have opinions to
share about these names for the attributes of pg_stat_checkpointer,
though.

- single row, containing global data for the cluster.
+ single row, containing data about the bgwriter of the cluster.

"bgwriter" is used in one place of the docs, so perhaps "background
writer" is better here?

The error message generated for an incorrect target needs to be
updated in pg_stat_reset_shared():
=# select pg_stat_reset_shared('checkpointe');
ERROR: 22023: unrecognized reset target: "checkpointe"
HINT: Target must be "archiver", "bgwriter", "io", "recovery_prefetch", or "wal".
--
Michael