On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <michael@paquier.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