At Thu, 04 Aug 2022 13:12:32 -0400, Reid Thompson
<reid.thomp...@crunchydata.com> wrote in
> On Fri, 2022-07-29 at 11:53 +0900, Kyotaro Horiguchi wrote:
> >
> > That makes the memorycontext-tree structure unstable because
> > CacheMemoryContext can be created on-the-fly.
> >
> > Honestly I don't like to call CreateCacheMemoryContext in the two
> > functions on-the-fly. Since every process that calls
> > pgstat_initialize() necessarily calls pgstat_setup_memcxt() at latest
> > at process termination, I think we can create at least
> > CacheMemoryContext in pgstat_initialize().
>
> Attached is a patch creating CacheMemoryContext() in pgstat_initialize()
> rather than the two previous patch locations.
>
> > Or couldn't we create the
> > all three contexts in the function, instead of calling
> > pgstat_setup_memcxt() on-the fly?
>
> You note that that pgstat_setup_memcxt() is called at latest at process
> termination -- was the intent to hold off on requesting memory for these
> two contexts until it was needed?
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.
Thus I thought that we may let pgstat_initialize() promptly allocate
the memory.
Does it make sense?
About the patch, I had something like the attached in my mind.
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..33539202a9 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -14,6 +14,7 @@
#include "pgstat.h"
#include "storage/shmem.h"
+#include "utils/catcache.h"
#include "utils/memutils.h"
#include "utils/pgstat_internal.h"
@@ -55,9 +56,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 +225,22 @@ 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);
+
+ CreateCacheMemoryContext();
+ pgStatSharedRefContext =
+ AllocSetContextCreate(CacheMemoryContext,
+ "PgStat Shared Ref",
+ ALLOCSET_SMALL_SIZES);
+ pgStatEntryRefHashContext =
+ AllocSetContextCreate(CacheMemoryContext,
+ "PgStat Shared Ref
Hash",
+ ALLOCSET_SMALL_SIZES);
}
void
@@ -407,7 +421,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 +983,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);
-}
