Re: Introduce a new view for checkpointer related stats

On Tue, Nov 22, 2022 at 1:26 PM Drouvot, Bertrand
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On 11/17/22 1:51 PM, Bharath Rupireddy wrote:
> > Hi,
> >
> > pg_stat_bgwriter view currently reports checkpointer stats as well. It
> > is that way because historically checkpointer was part of bgwriter
> > until the commits 806a2ae and bf405ba, that went into PG 9.2,
> > separated them out. I think it is time for us to separate checkpointer
> > stats to its own view. I discussed it in another thread [1] and it
> > seems like there's an unequivocal agreement for the proposal.
> >
> > I'm attaching a patch introducing a new pg_stat_checkpointer view,
> > with this change the pg_stat_bgwriter view only focuses on bgwriter
> > related stats. The patch does mostly mechanical changes. I'll add it
> > to CF in a bit.
> >
> > Thoughts?
>
> +1 for the dedicated view.
>
> + <para>
> + The <structname>pg_stat_checkpointer</structname> view will always have a
> + single row, containing global data for the cluster.
> + </para>
>
> what about "containing data about checkpointer activity of the cluster"? (to provide more "details" (even if that seems obvious given the name of the view) and be consistent with the pg_stat_wal description too).
> And if it makes sense to you, While at it, why not go for "containing data about bgwriter activity of the cluster" for pg_stat_bgwriter too?

Nice catch. Modified.

> +CREATE VIEW pg_stat_checkpointer AS
> + SELECT
> + pg_stat_get_timed_checkpoints() AS checkpoints_timed,
> + pg_stat_get_requested_checkpoints() AS checkpoints_req,
> + pg_stat_get_checkpoint_write_time() AS checkpoint_write_time,
> + pg_stat_get_checkpoint_sync_time() AS checkpoint_sync_time,
> + pg_stat_get_buf_written_checkpoints() AS buffers_checkpoint,
> + pg_stat_get_buf_written_backend() AS buffers_backend,
> + pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> + pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
>
> I don't think we should keep the checkpoints_ prefix (or _checkpoint suffix) in the column names now that they belong to a dedicated view (also the pg_stat_bgwriter view's columns don't have a
> bgwriter prefix/suffix).
>
> And while at it, I'm not sure the wal_ suffix in pg_stat_wal make sense too.
>
> The idea is to have consistent naming between the views and their columns: I'd vote without prefix/suffix.

-1. If the prefix is removed, some column names become unreadable -
timed, requested, write_time, sync_time, buffers. We might think of
renaming those columns to something more readable, I tend to not do
that as it can break largely the application/service layer/monitoring
tools, of course even with the new pg_stat_checkpointer view, we can't
avoid that, however the changes are less i.e. replace pg_stat_bgwriter
with the new view.

I'm attaching the v2 patch for further review.

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