Re: Introduce a new view for checkpointer related stats

Hi,

On 2023-02-09 19:00:00 +0530, Bharath Rupireddy wrote:
> On Thu, Feb 9, 2023 at 12:33 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2023-02-09 12:21:51 +0530, Bharath Rupireddy wrote:
> > > @@ -1105,18 +1105,22 @@ CREATE VIEW pg_stat_archiver AS
> > >
> > > CREATE VIEW pg_stat_bgwriter AS
> > > SELECT
> > > - pg_stat_get_bgwriter_timed_checkpoints() AS checkpoints_timed,
> > > - pg_stat_get_bgwriter_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_bgwriter_buf_written_checkpoints() AS buffers_checkpoint,
> > > pg_stat_get_bgwriter_buf_written_clean() AS buffers_clean,
> > > pg_stat_get_bgwriter_maxwritten_clean() AS maxwritten_clean,
> > > - pg_stat_get_buf_written_backend() AS buffers_backend,
> > > - pg_stat_get_buf_fsync_backend() AS buffers_backend_fsync,
> > > pg_stat_get_buf_alloc() AS buffers_alloc,
> > > pg_stat_get_bgwriter_stat_reset_time() AS stats_reset;
> > >
> > > +CREATE VIEW pg_stat_checkpointer AS
> > > + SELECT
> > > + pg_stat_get_timed_checkpoints() AS timed_checkpoints,
> > > + pg_stat_get_requested_checkpoints() AS requested_checkpoints,
> > > + 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_written_checkpoints,
> > > + pg_stat_get_buf_written_backend() AS buffers_written_backend,
> > > + pg_stat_get_buf_fsync_backend() AS buffers_fsync_backend,
> > > + pg_stat_get_checkpointer_stat_reset_time() AS stats_reset;
> > > +
> >
> > I don't think the backend written stats belong more accurately in
> > pg_stat_checkpointer than pg_stat_bgwriter.
>
> We accumulate buffers_written_backend and buffers_fsync_backend of all
> backends under checkpointer stats to show the aggregated results to
> the users. I think this is correct because the checkpointer is the one
> that processes fsync requests (of course, backends themselves can
> fsync when needed, that's what the buffers_fsync_backend shows),
> whereas bgwriter doesn't perform IIUC.

That's true for buffers_fsync_backend, but not true for
buffers_backend/buffers_written_backend.

That isn't tied to checkpointer.

I think if we end up breaking compat, we should just drop that column. The
pg_stat_io patch from Melanie, which I hope to finish committing by tomorrow,
provides that in a more useful way, in a less confusing place.

I'm not sure it's worth having buffers_fsync_backend in pg_stat_checkpointer
in that case. You can get nearly the same information from pg_stat_io as well
(except fsyncs for SLRUs that couldn't be put into the queue, which you'd not
see right now - hard to believe that ever happens at a relelvant frequency).

> > I continue to be worried about breaking just about any postgres monitoring
> > setup.
>
> Hm. Yes, it requires minimal and straightforward changes in monitoring
> scripts. Please note that we separated out bgwriter and checkpointer
> in v9.2 12 years ago but we haven't had a chance to separate the stats
> so far. We might do it at some point of time, IMHO this is that time.

> We did away with promote_trigger_file (cd4329d) very recently. The
> agreement was that the changes required to move on to other mechanisms
> of promotion are minimal, hence we didn't want it to be first
> deprecated and then removed.

That's not really comparable, because we have had pg_ctl promote for a long
time. You can use it across all supported versions. pg_promote() is nearly
there as well. Whereas there's no way to use same query across all versions.

IME there also exist a lot more hand-rolled monitoring setups
than hand-rolled automatic promotion setups.

> From the discussion upthread, it looks like Robert, Amit, Bertrand,
> Greg and myself are in favour of not having a deprecated version but
> moving them to the new pg_stat_checkpointer view.

Yep, and I think you are all wrong, and that this is just going to cause
unnecessary pain :). I'm not going to try to prevent the patch from going in
because of this, just to be clear.

Greetings,

Andres Freund