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

Combined responses to both Justin and Andres here.
v19 attached.

On Wed, Dec 15, 2021 at 5:38 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> +int backend_type_get_idx(BackendType backend_type)
> +BackendType idx_get_backend_type(int idx)
>
> => I think it'd be desirable for these to be either static functions (which
> won't work for your needs) or macros, or inline functions in the header.
>
> - if (strcmp(target, "archiver") == 0)
> + pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_RESETSHAREDCOUNTER);
> + if (strcmp(target, "buffers") == 0)

Done

>
> => This should be added in alphabetical order. Which is unimportant, but it
> will also makes the patch 2 lines shorter. The doc patch should also be in
> order.

Thanks for catching this.
I've corrected the order in most locations. The exception is in
pgstat_reset_shared_counters():

if (strcmp(target, "buffers") == 0)
{
msg.m_resettarget = RESET_BUFFERS;
pgstat_send_buffers_reset(&msg);
return;
}

Because "buffers" is a special case
which uses a different send function, I prefer to have it first.

>
> + * Don't count dead backends. They will be added below There are no
>
> => Missing a period.

Fixed.

On Thu, Dec 16, 2021 at 3:18 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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 added B_STANDALONE_BACKEND and set it in InitStandaloneBackend() (as
opposed to in PostgresSingleUserMain()) so that the bootstrap process
could also use it.

> > > > 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 tried this, but I actually don't think it is the right way to go. In
order to maintain the old view with the new source code, I had to add
new code to maintain a separate resets array just for the bgwriter view.
It adds some fiddly code that will be annoying to maintain (the reset
logic is confusing enough as is).
And, besides the implementation complexity, if a user resets
pg_stat_bgwriter and not pg_stat_buffers (or vice versa), they will
see totally different numbers for "buffers_backend" in pg_stat_bgwriter
than shared buffers written by B_BACKEND in pg_stat_buffers. I would
find that confusing.

Instead, what I did was create the separate pg_stat_checkpointer view
and move most of the old pg_stat_bgwriter stats over there.

Because that left us with a pg_stat_bgwriter view with one column, I
added a few stats to it which could later be expanded.

In pg_stat_bgwriter, I renamed "maxwritten_clean" to "rounds_hit_limit".
I added "rounds_cleaned_estimate" and "rounds_lapped_clock" which are
the other two exit conditions from the LRU scan loop in BgBufferSync().

There are other stats related to bgwriter that might be more interesting
(e.g. number of times bgwriter was woken up to clean, % of time bgwriter
spends in hibernation vs cleaning, etc); however the stats I ended up
adding were available in the same scope as maxwritten_clean and seemed
like a non-intrusive way to start building out pg_stat_bgwriter.

BgBufferSync() has a *lot* of local variables that are all getting
incremented and reset in a complicated way, so I'm not 100% sure that
the new stats I added are actually correct.

> > 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

I noticed that the po files for pgstat.c are not updated (the msgid I am
concerned with has the old line number in pgstat.c and the old message).
So, I tried running `make update-po`, but it didn't have the documented
effect:

"to be called if the messages in the program source have changed, in
order to merge the changes into the existing .po files"

No po.new files were created.

I can look into it more, though if it is part of an external workflow, as
Álvaro suggested, perhaps I shouldn't?

- Melanie