At Sat, 6 Aug 2022 19:19:39 -0700, Andres Freund <and...@anarazel.de> wrote in > Hi, > > On 2022-08-05 17:22:38 +0900, Kyotaro Horiguchi wrote: > > I think it a bit different. Previously that memory (but for a bit > > different use, precisely) was required only when stats data is read so > > almost all server processes didn't need it. Now, every server process > > that uses pgstats requires the two memory if it is going to write > > stats. Even if that didn't happen until process termination, that > > memory eventually required to flush possibly remaining data. That > > final write might be avoidable but I'm not sure it's worth the > > trouble. As the result, calling pgstat_initialize() is effectively > > the declaration that the process requires the memory. > > I don't think every process will end up calling pgstat_setup_memcxt() - > e.g. walsender, bgwriter, checkpointer probably don't? What do we gain by > creating the contexts eagerly?
Yes. they acutally does, in shmem_shutdown hook function, during at-termination stats write. I didn't consider to make that not happen, to save 2kB of memory on such small number of processes. > > Thus I thought that we may let pgstat_initialize() promptly allocate > > the memory. > > That makes some sense - but pgstat_attach_shmem() seems like a very strange > place for the call to CreateCacheMemoryContext(). Sure. (I hesitantly added #include for catcache.h..) > I wonder if we shouldn't just use TopMemoryContext as the parent for most of > these contexts instead. CacheMemoryContext isn't actually a particularly good > fit anymore. It looks better than creating CacheMemoryContext. Now pgstat_initialize() creates the memory contexts for pgstats use under TopMemoryContext. And we don't hastle to avoid maybe-empty at-process-termination writes.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index 89060ef29a..e24a19882b 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -55,9 +55,6 @@ static void pgstat_release_all_entry_refs(bool discard_pending); typedef bool (*ReleaseMatchCB) (PgStat_EntryRefHashEntry *, Datum data); static void pgstat_release_matching_entry_refs(bool discard_pending, ReleaseMatchCB match, Datum match_data); -static void pgstat_setup_memcxt(void); - - /* parameter for the shared hash */ static const dshash_parameters dsh_params = { sizeof(PgStat_HashKey), @@ -227,6 +224,21 @@ pgstat_attach_shmem(void) pgStatLocal.shmem->hash_handle, 0); MemoryContextSwitchTo(oldcontext); + + /* + * memory contexts needed by both reads and writes on stats shared memory + */ + Assert(pgStatSharedRefContext == NULL); + Assert(pgStatEntryRefHashContext == NULL); + + pgStatSharedRefContext = + AllocSetContextCreate(TopMemoryContext, + "PgStat Shared Ref", + ALLOCSET_SMALL_SIZES); + pgStatEntryRefHashContext = + AllocSetContextCreate(TopMemoryContext, + "PgStat Shared Ref Hash", + ALLOCSET_SMALL_SIZES); } void @@ -407,7 +419,6 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, Oid objoid, bool create, Assert(pgStatLocal.shared_hash != NULL); Assert(!pgStatLocal.shmem->is_shutdown); - pgstat_setup_memcxt(); pgstat_setup_shared_refs(); if (created_entry != NULL) @@ -970,18 +981,3 @@ pgstat_reset_entries_of_kind(PgStat_Kind kind, TimestampTz ts) { pgstat_reset_matching_entries(match_kind, Int32GetDatum(kind), ts); } - -static void -pgstat_setup_memcxt(void) -{ - if (unlikely(!pgStatSharedRefContext)) - pgStatSharedRefContext = - AllocSetContextCreate(CacheMemoryContext, - "PgStat Shared Ref", - ALLOCSET_SMALL_SIZES); - if (unlikely(!pgStatEntryRefHashContext)) - pgStatEntryRefHashContext = - AllocSetContextCreate(CacheMemoryContext, - "PgStat Shared Ref Hash", - ALLOCSET_SMALL_SIZES); -}