Re: shared-memory based stats collector

Hi,

On 2021-03-15 19:04:29 -0700, Andres Freund wrote:
> On 2021-03-13 10:05:21 -0800, Andres Freund wrote:
> > Cool. I'll give it a try.

Ooops, I was intending to write a bit about my attempt at that:

I did roughly the first steps of the split as I had outlined. I moved:

1) wait event related functions into utils/activity/wait_event.c /
wait_event.h

2) "backend status" functionality (PgBackendStatus stuff) into
utils/activity/backend_status.c

3) "progress" related functionality into
utils/activity/backend_progress.c

I think 1 and 2 are good (albeit in need of further polish). I'm a bit
less sure about 3:
- There's dependency from backend_status.h to backend_progress.h,
because it PGSTAT_NUM_PROGRESS_PARAM etc.
- it's a fairly small amount of code
- there's potential for confusion, because there's also
include/commands/progress.h

On balance I think 3) is probably worth it, but I'm far from confident.

Happy to bikeshed about the names for a moment...

Questions / Points:

- I'm inclined to leave pgstat_report_{activity, tmpfile, appname,
timestamp, ..} alone naming-wise, but to rename pgstat_bestart() to
something like pgbestat_start()?

- I've not gone through all the files that could now remove pgstat.h,
replacing it with wait_event.h - I'm thinking it might be worth
waiting till just after code freeze with that (there'll new additions,
and it's likely to cause conflicts)?

- backend_status.h needs miscadmin.h, due to BackendType. Imo that's a
header we should try to avoid exposing in headers if possible. But
right now there's no good place to move BackendType to. So I'd let
this slide for now.

On 2021-03-15 19:04:29 -0700, Andres Freund wrote:
> I have a few questions about the patch:
> - Why was collect_oids() changed to a different hashtable as part of
> this change? Seems fairly independent?
>
> - What's the point of all those cached_* stuff? There's not a single
> comment explaining it as far as I can tell...
>
> Several of them are never used as a cache! E.g. cached_archiverstats,
> cached_bgwriterstats, ...
>
> - What is the idea behind pgstat_reset_shared_counters() using
> pgstat_copy_global_stats() to reset, using StatsShmem->*_reset_offset?
> But then still taking a lock in pgstat_fetch_stat_*? Again, no
> comments explaining what the goal is.
>
> It kinda looks like you tried to make both read and write paths not
> use the lock, but then ended up using a lock?
>
>
> Do you have some benchmarks that you used to verify performance?
>
>
> I think I'm going to try to split the storage of fixed-size stats in
> StatsShmemStruct into a separate patch. That's already a pretty large
> change, and it's pretty much unrelated to the rest.

Some more:
- pgstat_vacuum_stat() talks about using three phases, one with only a
shared lock. That doesn't seem to exist (anymore)?

- WAIT_EVENT_PGSTAT_MAIN was renamed to WAIT_EVENT_READING_STATS_FILE. I
assume that was intended to be used in pgstat_read_statsfile(),
pgstat_write_statsfile()? Not sure there's much point in adding it,
because at that stage nobody can observe wait events anyway...

- There are a lot of diffs like
@@ -1267,7 +1264,7 @@ pg_stat_get_db_xact_commit(PG_FUNCTION_ARGS)
if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
result = 0;
else
- result = (int64) (dbentry->n_xact_commit);
+ result = (int64) (dbentry->counts.n_xact_commit);

PG_RETURN_INT64(result);
}

I wonder if we should instead just return *entry->counts from
pgstat_fetch_stat_* (adjusting the names). There's as far as I can
tell never a reason for pgstatfuncs.c (and others if there are) to
need the remaining members? That'd could the patch down a lot I
think?

What's especially nice is that afterwards PgStat_StatEntryHeader
wouldn't need to be exposed anymore, and in turn, atomic.h wouldn't
need to be included anymore.

- pgstat.h had dshash_table_handle members in PgStat_StatDBEntry, I
assume those were leftovers from an older version, not something you
forsee needing?

- There's a pretty weird change in relcache.c:
+ /* break mutual link with stats entry */
+ pgstat_delinkstats(relation);
+
+ if (relation->rd_rel)

did you add that if intentionally?

- Is there any reason to treat PgStat_ReplSlot as something of a dynamic
number, instead of just having a fixed number of slots in
StatsShmemStruct? The GUC can't change without a restart...

Greetings,

Andres Freund