Re: shared-memory based stats collector - v68

Hi,

On 2022-04-04 13:45:40 -0700, David G. Johnston wrote:
> I didn't take the time to fixup all the various odd typos in the general
> code comments; none of them reduced comprehension appreciably. I may do so
> when/if I do another pass.

Cool.

> My first encounter with pg_stat_exists_stat() didn't draw my attention as
> being problematic so I'd say we just stick with it. As a SQL user reading:
> WHERE exists (...) is somewhat natural; using "have" or back-to-back
> stat_stat is less appealing.

There are a number of other *_exists functions, albeit not within
pg_stat_*. Like jsonb_exists. Perhaps just pg_stat_exists()?

> I would suggest we do away with stats_fetch_consistency "snapshot" mode and
> instead add a function that can be called that would accomplish the same
> thing but in "cache" mode. Future iterations of that function could accept
> patterns, allowing for something between "one" and "everything".

I don't want to do that. We had a lot of discussion around what consistency
model we want, and Tom was adamant that there needs to be a mode that behaves
like the current consistency model (which is what snapshot behaves like, with
very minor differences). A way to get back to the old behaviour seems good,
and the function idea doesn't provide that.

(merged the typos that I hadn't already fixed based on Justin / Thomas'
feedback)

> @@ -371,7 +371,9 @@ pgstat_discard_stats(void)
> /*
> * pgstat_before_server_shutdown() needs to be called by exactly one
> process
> * during regular server shutdowns. Otherwise all stats will be lost.
> - *
> + * XXX: What bad things happen if this is invoked by more than one process?
> + * I'd presume stats are not actually lost in that case. Can we just
> 'no-op'
> + * subsequent calls and say "at least once at shutdown, as late as
> possible"

What's the reason behind this question? There really shouldn't be a second
call (and there's only a single callsite). As is you'd get an assertion
failure about things already having been shutdown.

I don't think we want to relax that, because in all the realistic scenarios
that I can think of that'd open us up to loosing stats that were generated
after the first writeout of the stats data.

You mentioned this as a restriction above - I'm not seeing it as such? I'd
like to write out stats more often in the future (e.g. in the checkpointer),
but then it'd not be written out with this function...

> @@ -654,6 +656,14 @@ pgstat_reset_single_counter(PgStat_Kind kind, Oid
> objoid)
>
> Assert(!pgstat_kind_info_for(kind)->fixed_amount);
>
> + /*
> + * More of a conceptual observation here - the fact that something is
> fixed does not imply
> + * that it is not fixed at a value greater than zero and thus could have
> single subentries
> + * that could be addressed.

pgstat_reset_single_counter() is a pre-existing function (with a pre-existing
name, but adapted signature in the patch), it's currently only used for
functions and relation stats.

> + * I also am unsure, off the top of my head, whether both replication
> slots and subscriptions,
> + * which are fixed, can be reset singly (today, and/or whether this patch
> enables that capability)
> + */

FWIW, neither are implemented as fixed amount stats. There's afaics no limit
at all for the number of existing subscriptions (although some would either
need to be disabled or you'd get errors). While there is a limit on the number
of slots, that's a configurable limit. So replication slot stats are also
implemented as variable amount stats (that used to be different, wasn't nice).

There's one example of fixed amount stats that can be reset more granularly,
namely slru. That can be done via pg_stat_reset_slru().

Thanks,

Andres