On Thu, Oct 26, 2023 at 10:55:00PM +0530, Bharath Rupireddy wrote: > On Thu, Oct 26, 2023 at 7:30 AM Michael Paquier <mich...@paquier.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
signature.asc
Description: PGP signature