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 <melanieplage...@gmail.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