Re: Introduce a new view for checkpointer related stats

On Tue, Nov 22, 2022 at 1:26 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.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
seemsobvious 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"
forpg_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
toa 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