Horton

Re: shared-memory based stats collector - v70

Drouvot, Bertrand <bdrouvot@amazon.com>

View thread View on postgr.es
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
To: Greg Stark <stark@mit.edu>
Cc: Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Melanie Plageman <melanieplageman@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Justin Pryzby <pryzby@telsasoft.com>, Thomas Munro <thomas.munro@gmail.com>, "David G. Johnston" <david.g.johnston@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-08-16T12:49:04Z
Lists: pgsql-hackers 
Hi,

On 8/15/22 4:46 PM, Greg Stark wrote:
> On Thu, 11 Aug 2022 at 02:11, Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> As Andres was not -1 about that idea (as it should be low cost to add
>> and maintain) as long as somebody cares enough to write something: then
>> I'll give it a try and submit a patch for it.
> I agree it would be a useful feature. I think there may be things to
> talk about here though.
>
> 1) Are you planning to go through the local hash table and
> LocalSnapshot and obey the consistency mode? I was thinking a flag
> passed to build_snapshot to request global mode might be sufficient
> instead of a completely separate function.

I think the new API should behave as PGSTAT_FETCH_CONSISTENCY_NONE (as 
querying from all the databases increases the risk of having to deal 
with "large" number of objects).

I've in mind to do something along those lines (still need to add some 
filtering, extra check on the permission,...):

+       dshash_seq_init(&hstat, pgStatLocal.shared_hash, false);
+       while ((p = dshash_seq_next(&hstat)) != NULL)
+       {
+               Datum values[PG_STAT_GET_ALL_TABLES_STATS_COLS];
+               bool nulls[PG_STAT_GET_ALL_TABLES_STATS_COLS];
+               PgStat_StatTabEntry * tabentry = NULL;
+               MemSet(values, 0, sizeof(values));
+               MemSet(nulls, false, sizeof(nulls));
+

+               if (p->key.kind != PGSTAT_KIND_RELATION)
+                       continue;

+               if (p->dropped)
+                       continue;
+
+               stats_data = dsa_get_address(pgStatLocal.dsa, p->body);
+               LWLockAcquire(&stats_data->lock, LW_SHARED);
+               tabentry = pgstat_get_entry_data(PGSTAT_KIND_RELATION, 
stats_data);
+
+
+               values[0] = ObjectIdGetDatum(p->key.dboid);
+               values[1] = ObjectIdGetDatum(p->key.objoid);
+               values[2]= DatumGetInt64(tabentry->tuples_inserted);

.

.

+               tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, 
values, nulls);
+               LWLockRelease(&stats_data->lock);
+       }
+       dshash_seq_term(&hstat);

What do you think?

> 2) When I did the function attached above I tried to avoid returning
> the whole set and make it possible to process them as they arrive.

Is it the way it has been done? (did not look at your function yet)

> I
> actually was hoping to get to the point where I could start shipping
> out network data as they arrive and not even buffer up the response,
> but I think I need to be careful about hash table locking then.

If using dshash_seq_next() the already returned elements are locked.

But I guess you would like to unlock them (if you are able to process 
them as they arrive)?

> 3) They key difference here is that we're returning whatever stats are
> in the hash table rather than using the catalog to drive a list of id
> numbers to look up.

Right.

> I guess the API should make it clear this is what
> is being returned

Right. I think we'll end up with a set of relations id (not their names) 
and their associated stats.

> -- on that note I wonder if I've done something
> wrong because I noted a few records with InvalidOid where I didn't
> expect it.

It looks like that InvalidOid for the dbid means that the entry is for a 
shared relation.

Where did you see them (while not expecting them)?

> 4) I'm currently looping over the hash table returning the records all
> intermixed. Some users will probably want to do things like "return
> all Relation records for all databases" or "return all Index records
> for database id xxx". So some form of filtering may be best or perhaps
> a way to retrieve just the keys so they can then be looked up one by
> one (through the local cache?).

I've in mind to add some filtering on the dbid (I think it could be 
useful for monitoring tool with a persistent connection to one database 
but that wants to pull the stats database per database).

I don't think a look up through the local cache will work if the 
entry/key is related to another database the API is launched from.

> 5) On that note I'm not clear how the local cache will interact with
> these cross-database lookups. That should probably be documented...

yeah I don't think that would work (if by local cache you mean what is 
in the relcache).

Regards,

-- 
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com

Commits

  1. Improve comment about dropped entries in pgstat.c

  2. Fix temporary memory leak in system table index scans

  3. pgstat: set timestamps of fixed-numbered stats after a crash.

  4. pgstat: Update docs to match the shared memory stats reality.

  5. pgstat: Hide instability in stats.spec with -DCATCACHE_FORCE_RELEASE.

  6. pgstat: add/extend tests for resetting various kinds of stats.

  7. Add minimal tests for recovery conflict handling.

  8. pgstat: test stats interactions with physical replication.

  9. pgstat: add tests for handling of restarts, including crashes.

  10. pgstat: add tests for transaction behaviour, 2PC, function stats.

  11. pgstat: add pg_stat_have_stats() test helper.

  12. pgstat: add pg_stat_force_next_flush(), use it to simplify tests.

  13. pgstat: move pgstat.c to utils/activity.

  14. pgstat: store statistics in shared memory.

  15. pgstat: remove stats_temp_directory.

  16. pgstat: rename STATS_COLLECTOR GUC group to STATS_CUMULATIVE.

  17. pgstat: revise replication slot API in preparation for shared memory stats.

  18. pgstat: scaffolding for transactional stats creation / drop.

  19. pgstat: introduce PgStat_Kind enum.

  20. pgstat: prepare APIs used by pgstatfuncs for shared memory stats.

  21. pgstat: add pgstat_copy_relation_stats().

  22. pgstat: rename some pgstat_send_* functions to pgstat_report_*.

  23. pgstat: stats collector references in comments.

  24. pgstat: move transactional code into pgstat_xact.c.

  25. dsm: allow use in single user mode.

  26. dshash: revise sequential scan support.

  27. pgstat: remove some superflous comments from pgstat.h.

  28. pgstat: reorder pgstat.[ch] contents.

  29. pgstat: split different types of stats into separate files.

  30. pgstat: introduce pgstat_relation_should_count().

  31. pgstat: separate "xact level" handling out of relation specific functions.

  32. pgstat: rename pgstat_initstats() to pgstat_relation_init().

  33. pgstat: split out WAL handling from pgstat_{initialize,report_stat}.

  34. pgstat: run pgindent on pgstat.c/h.

  35. pgstat: split relation, database handling out of pgstat_report_stat().

  36. Move code around in StartupXLOG().

  37. pgstat: Prepare to use mechanism for truncated rels also for droppped rels.

  38. pgstat: Split out relation stats handling from AtEO[Sub]Xact_PgStat() etc.

  39. pgstat: Schedule per-backend pgstat shutdown via before_shmem_exit().

  40. Schedule ShutdownXLOG() in single user mode using before_shmem_exit().

  41. Make parallel worker shutdown complete entirely via before_shmem_exit().

  42. pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.

  43. pgstat: split reporting/fetching of bgwriter and checkpointer stats.

  44. Split backend status and progress related functionality out of pgstat.c.

  45. Split wait event related code from pgstat.[ch] into wait_event.[ch].

  46. Make archiver process an auxiliary process.

  47. Force to send remaining WAL stats to the stats collector at walwriter exit.

  48. Add pg_stat_database counters for sessions and session time

  49. Collect statistics about SLRU caches

  50. Don't run atexit callbacks in quickdie signal handlers.

  51. Create a "fast path" for acquiring weak relation locks.