Re: shared-memory based stats collector - Mailing list pgsql-hackers

At Tue, 08 Sep 2020 17:55:57 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Tue, 08 Sep 2020 17:01:47 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > At Thu, 3 Sep 2020 13:16:59 -0400, Stephen Frost <sfrost@snowman.net> wrote in> > I've looked through (some of)
thisthread and through the patches also
 
> > > and hope to provide a review of the bits that should be targetting v14
> > > (unlike the above) soon.
> > 
> > Thanks. Current the patch is found to lead to write contention heavier
> > than the current stats collector when nearly a thousand backend
> > heavily write to the same table. I need to address that.
> > 
> > - Collect stats via sockets (in the same way as the current implement)
> >   and write/read to/from shared memory.
> > 
> > - Use our own lock-free (maybe) ring-buffer before stats-write enters
> >   lock-waiting mode, then new stats collector(!) process consumes the
> >   queue.
> > 
> > - Some other measures..

- Make dshash search less frequent. To find the actual source of the
  contension, We're going to measure performance with the attached on
  top of the latest patch let sessions cache the result of dshash
  searches for the session lifetime. (table-dropping vacuum clears the
  local hash.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0ce7b7a891326a3fdcf85b001586f721bef569f9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Wed, 9 Sep 2020 16:44:57 +0900
Subject: [PATCH v36 8/8] Experimental local cache for dshash.

This patch suffers LWLock contension than unpatched while very many
sessions commits frequently. As an effort to identify the source, this
patch caches dshash information in a local hash to avoid dshash locks.
---
 src/backend/postmaster/pgstat.c | 67 +++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index b07add0a4f..caa067e9fe 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -35,6 +35,7 @@
 #include "access/xact.h"
 #include "catalog/pg_database.h"
 #include "catalog/pg_proc.h"
+#include "common/hashfn.h"
 #include "libpq/libpq.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -112,6 +113,8 @@ typedef struct StatsShmemStruct
     dsa_pointer slru_stats;        /* Ditto for SLRU stats */
     int            refcount;        /* # of processes that is attaching the shared
                                  * stats memory */
+    pg_atomic_uint64 del_ent_count; /* # of entries deleted. not protected by
+                                       StatsLock */
 }            StatsShmemStruct;
 
 /* BgWriter global statistics counters */
@@ -254,6 +257,7 @@ typedef struct PgStatSharedSLRUStats
 typedef struct PgStatLocalHashEntry
 {
     PgStatHashEntryKey key;        /* hash key */
+    char               status;    /* for simplehash use */
     PgStatEnvelope *env;        /* pointer to stats envelope in heap */
 }            PgStatLocalHashEntry;
 
@@ -280,6 +284,18 @@ static const dshash_parameters dsh_rootparams = {
     LWTRANCHE_STATS
 };
 
+/* define hashtable for dshash caching */
+#define SH_PREFIX pgstat_cache
+#define SH_ELEMENT_TYPE PgStatLocalHashEntry
+#define SH_KEY_TYPE PgStatHashEntryKey
+#define SH_KEY key
+#define SH_HASH_KEY(tb, key) hash_bytes((unsigned char *)&key, sizeof(PgStatHashEntryKey))
+#define SH_EQUAL(tb, a, b) (memcmp(&a, &b, sizeof(PgStatHashEntryKey)) == 0)
+#define SH_SCOPE static inline
+#define SH_DEFINE
+#define SH_DECLARE
+#include "lib/simplehash.h"
+
 /* The shared hash to index activity stats entries. */
 static dshash_table *pgStatSharedHash = NULL;
 
@@ -326,6 +342,8 @@ typedef struct TwoPhasePgStatRecord
 } TwoPhasePgStatRecord;
 
 /* Variables for backend status snapshot */
+static pgstat_cache_hash *pgStatEntHash = NULL;
+static int     pgStatEntHashAge = 0;
 static MemoryContext pgStatLocalContext = NULL;
 static MemoryContext pgStatSnapshotContext = NULL;
 static HTAB *pgStatSnapshotHash = NULL;
@@ -437,6 +455,7 @@ StatsShmemInit(void)
         Assert(!found);
 
         StatsShmem->stats_dsa_handle = DSM_HANDLE_INVALID;
+        pg_atomic_init_u64(&StatsShmem->del_ent_count, 0);
     }
 }
 
@@ -1432,6 +1451,9 @@ pgstat_vacuum_stat(void)
 
         delete_stat_entry(p->type, p->databaseid, p->objectid, true);
     }
+
+    if (nvictims > 0)
+        pg_atomic_add_fetch_u64(&StatsShmem->del_ent_count, 1);
 }
 
 
@@ -1493,6 +1515,8 @@ pgstat_drop_database(Oid databaseid)
         delete_stat_entry((*p)->type, (*p)->databaseid, (*p)->objectid, true);
 
     pfree(envlist);
+
+    pg_atomic_add_fetch_u64(&StatsShmem->del_ent_count, 1);
 }
 
 
@@ -5119,6 +5143,7 @@ get_stat_entry(PgStatTypes type, Oid dbid, Oid objid,
 {
     bool        create = (initfunc != NULL);
     PgStatHashEntry *shent;
+    PgStatLocalHashEntry *loent;
     PgStatEnvelope *shenv = NULL;
     PgStatHashEntryKey key;
     bool        myfound;
@@ -5128,6 +5153,31 @@ get_stat_entry(PgStatTypes type, Oid dbid, Oid objid,
     key.type = type;
     key.databaseid = dbid;
     key.objectid = objid;
+
+    if (pgStatEntHash)
+    {
+        uint64 currage;
+
+        currage = pg_atomic_read_u64(&StatsShmem->del_ent_count);
+
+        if (currage == pgStatEntHashAge)
+        {
+            loent = pgstat_cache_lookup(pgStatEntHash, key);
+
+            if (loent)
+            {
+                if (found)
+                    *found = true;
+                return loent->env;
+            }
+        }
+        else
+        {
+            pgstat_cache_destroy(pgStatEntHash);
+            pgStatEntHash = NULL;
+        }
+    }
+
     shent = dshash_find_extended(pgStatSharedHash, &key,
                                  create, nowait, create, &myfound);
     if (shent)
@@ -5159,6 +5209,23 @@ get_stat_entry(PgStatTypes type, Oid dbid, Oid objid,
             shenv = dsa_get_address(area, shent->env);
 
         dshash_release_lock(pgStatSharedHash, shent);
+
+        /* register to local hash if possible */
+        if (pgStatEntHash || CacheMemoryContext)
+        {
+            if (pgStatEntHash == NULL)
+            {
+                pgStatEntHash =
+                    pgstat_cache_create(CacheMemoryContext,
+                                        PGSTAT_TABLE_HASH_SIZE, NULL);
+                pgStatEntHashAge =
+                    pg_atomic_read_u64(&StatsShmem->del_ent_count);
+            }
+    
+            loent = pgstat_cache_insert(pgStatEntHash, key, &myfound);
+            Assert(!myfound);
+            loent->env = shenv;
+        }
     }
 
     if (found)
-- 
2.18.4