At Sat, 6 Aug 2022 19:19:39 -0700, Andres Freund <[email protected]> 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);
-}
