Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Hi,

On 2021-12-15 16:40:27 -0500, Melanie Plageman wrote:
> > > +/*
> > > + * Before exiting, a backend sends its IO op statistics to the collector so
> > > + * that they may be persisted.
> > > + */
> > > +void
> > > +pgstat_send_buffers(void)
> > > +{
> > > + PgStat_MsgIOPathOps msg;
> > > +
> > > + PgBackendStatus *beentry = MyBEEntry;
> > > +
> > > + /*
> > > + * Though some backends with type B_INVALID (such as the single-user mode
> > > + * process) do initialize and increment IO operations stats, there is no
> > > + * spot in the array of IO operations for backends of type B_INVALID. As
> > > + * such, do not send these to the stats collector.
> > > + */
> > > + if (!beentry || beentry->st_backendType == B_INVALID)
> > > + return;
> >
> > Why does single user mode use B_INVALID? That doesn't seem quite right.
>
> I think PgBackendStatus->st_backendType is set from MyBackendType which
> isn't set for the single user mode process. What BackendType would you
> expect to see?

Either B_BACKEND or something new like B_SINGLE_USER_BACKEND?

> I also thought about having pgstat_sum_io_path_ops() return a value to
> indicate if everything was 0 -- which could be useful to future callers
> potentially?
>
> I didn't do this because I am not sure what the return value would be.
> It could be a bool and be true if any IO was done and false if none was
> done -- but that doesn't really make sense given the function's name it
> would be called like
> if (!pgstat_sum_io_path_ops())
> return
> which I'm not sure is very clear

Yea, I think it's ok to not do something fancier here for nwo.

> > > From 9f22da9041e1e1fbc0ef003f5f78f4e72274d438 Mon Sep 17 00:00:00 2001
> > > From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> > > Date: Wed, 24 Nov 2021 12:20:10 -0500
> > > Subject: [PATCH v17 6/7] Remove superfluous bgwriter stats
> > >
> > > Remove stats from pg_stat_bgwriter which are now more clearly expressed
> > > in pg_stat_buffers.
> > >
> > > TODO:
> > > - make pg_stat_checkpointer view and move relevant stats into it
> > > - add additional stats to pg_stat_bgwriter
> >
> > When do you think it makes sense to tackle these wrt committing some of the
> > patches?
>
> Well, the new stats are a superset of the old stats (no stats have been
> removed that are not represented in the new or old views). So, I don't
> see that as a blocker for committing these patches.

> Since it is weird that pg_stat_bgwriter had mostly checkpointer stats,
> I've edited this commit to rename that view to pg_stat_checkpointer.

> I have not made a separate view just for maxwritten_clean (presumably
> called pg_stat_bgwriter), but I would not be opposed to doing this if
> you thought having a view with a single column isn't a problem (in the
> event that we don't get around to adding more bgwriter stats right
> away).

How about keeping old bgwriter values in place in the view , but generated
from the new stats stuff?

> I noticed after changing the docs on the "bgwriter" target for
> pg_stat_reset_shared to say "checkpointer", that it still said "bgwriter" in
> src/backend/po/ko.po
> src/backend/po/it.po
> ...
> I presume these are automatically updated with some incantation, but I wasn't
> sure what it was nor could I find documentation on this.

Yes, they are - and often some languages lag updating things. There's a bit
of docs at https://www.postgresql.org/docs/devel/nls.html

Greetings,

Andres Freund