Re: Introduce a new view for checkpointer related stats

On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> I was looking at this patch, and got a few comments.

Thanks.

> 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?

+1. PSA v9-0003.

> + * Unlike the checkpoint fields, reqquests related fields are protected by
>
> s/reqquests/requests/.

Fixed.

> 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?

+1. I included the fix in a separate patch 0002 here.

> 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.

PSA v9 patch set.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com