Re: pgstat_send_connstats() introduces unnecessary timestamp and UDP overhead - Mailing list pgsql-hackers

At Wed, 25 Aug 2021 13:21:52 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Wed, 25 Aug 2021 12:51:58 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> > - Throttle the frequency where the connection stat packages are sent,
> > as of [2].
> > 
> > Magnus, this open item is assigned to you as the committer of
> > 960869d.  Could you comment on those issues?
> > 
> > [1]: https://www.postgresql.org/message-id/4095ceb328780d1bdba77ac6d9785fd7577ed047.camel@cybertec.at
> > [2]: https://www.postgresql.org/message-id/20210801205501.nyxzxoelqoo4x2qc@alap3.anarazel.de
> 
> About [2], we need to maintain session active/idel times on additional
> menbers in backend status.  Letting gpgstat_report_activity to
> directly scribble on backend status array would work?

So the attached is roughly that (just a PoC, of course).

- accumulate active and idle time on backend status array.
  (pgstat_report_activity)

- pgstat_get_db_session_time() and friends read pgstat file then sum
  up relevant members in backend status array. (So it scans on the
  array for every number of every database X().

- The function pgstat_send_connstats is called only at the end of a
  connection.  It reads backend status element then send the numbers
  to stats collector.  pgstat_shutdown_hook needs to be moved from
  on_shmem_exit to before_shmem_exit to read MyBEEntry.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7fcc3f6ded..47973f1e30 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -244,8 +244,6 @@ static int    pgStatXactCommit = 0;
 static int    pgStatXactRollback = 0;
 PgStat_Counter pgStatBlockReadTime = 0;
 PgStat_Counter pgStatBlockWriteTime = 0;
-PgStat_Counter pgStatActiveTime = 0;
-PgStat_Counter pgStatTransactionIdleTime = 0;
 SessionEndType pgStatSessionEndCause = DISCONNECT_NORMAL;
 
 /* Record that's written to 2PC state file when pgstat state is persisted */
@@ -323,7 +321,7 @@ static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg);
 static void pgstat_send_funcstats(void);
 static void pgstat_send_slru(void);
 static HTAB *pgstat_collect_oids(Oid catalogid, AttrNumber anum_oid);
-static void pgstat_send_connstats(bool disconnect, TimestampTz last_report);
+static void pgstat_send_connstats(TimestampTz now);
 
 static PgStat_TableStatus *get_tabstat_entry(Oid rel_id, bool isshared);
 
@@ -876,10 +874,8 @@ pgstat_report_stat(bool disconnect)
         return;
 
     /* for backends, send connection statistics */
-    if (MyBackendType == B_BACKEND)
-        pgstat_send_connstats(disconnect, last_report);
-
-    last_report = now;
+    if (MyBackendType == B_BACKEND && disconnect)
+        pgstat_send_connstats(now);
 
     /*
      * Destroy pgStatTabHash before we start invalidating PgStat_TableEntry
@@ -1368,39 +1364,41 @@ pgstat_drop_relation(Oid relid)
  * pgstat_send_connstats() -
  *
  *    Tell the collector about session statistics.
- *    The parameter "disconnect" will be true when the backend exits.
- *    "last_report" is the last time we were called (0 if never).
  * ----------
  */
 static void
-pgstat_send_connstats(bool disconnect, TimestampTz last_report)
+pgstat_send_connstats(TimestampTz now)
 {
     PgStat_MsgConn msg;
-    long        secs;
-    int            usecs;
+    volatile PgBackendStatus *beentry = MyBEEntry;
 
     if (pgStatSock == PGINVALID_SOCKET || !pgstat_track_counts)
         return;
 
     pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_CONNECTION);
     msg.m_databaseid = MyDatabaseId;
+    msg.m_disconnect = pgStatSessionEndCause;
 
-    /* session time since the last report */
-    TimestampDifference(((last_report == 0) ? MyStartTimestamp : last_report),
-                        GetCurrentTimestamp(),
-                        &secs, &usecs);
-    msg.m_session_time = secs * 1000000 + usecs;
+    msg.m_count = 1;
 
-    msg.m_disconnect = disconnect ? pgStatSessionEndCause : DISCONNECT_NOT_YET;
+    if (pgstat_track_activities && beentry)
+    {
+        msg.m_session_time =
+            (PgStat_Counter)(now - beentry->st_proc_start_timestamp) * 1000;
+        msg.m_active_time = beentry->st_session_active_time;
+        msg.m_idle_in_xact_time = beentry->st_session_idle_time;
 
-    msg.m_active_time = pgStatActiveTime;
-    pgStatActiveTime = 0;
-
-    msg.m_idle_in_xact_time = pgStatTransactionIdleTime;
-    pgStatTransactionIdleTime = 0;
-
-    /* report a new session only the first time */
-    msg.m_count = (last_report == 0) ? 1 : 0;
+        PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+        beentry->st_session_active_time = 0;
+        beentry->st_session_idle_time = 0;
+        PGSTAT_END_WRITE_ACTIVITY(beentry);
+    }
+    else
+    {
+        msg.m_session_time = 0;
+        msg.m_active_time = 0;
+        msg.m_idle_in_xact_time = 0;
+    }
 
     pgstat_send(&msg, sizeof(PgStat_MsgConn));
 }
@@ -2877,7 +2875,7 @@ pgstat_initialize(void)
     prevWalUsage = pgWalUsage;
 
     /* Set up a process-exit hook to clean up */
-    on_shmem_exit(pgstat_shutdown_hook, 0);
+    before_shmem_exit(pgstat_shutdown_hook, 0);
 }
 
 /* ------------------------------------------------------------
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index e19c4506ef..0f0eecc3b4 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -336,6 +336,8 @@ pgstat_bestart(void)
     lbeentry.st_activity_start_timestamp = 0;
     lbeentry.st_state_start_timestamp = 0;
     lbeentry.st_xact_start_timestamp = 0;
+    lbeentry.st_session_active_time = 0;
+    lbeentry.st_session_idle_time = 0;
     lbeentry.st_databaseid = MyDatabaseId;
 
     /* We have userid for client-backends, wal-sender and bgworker processes */
@@ -590,9 +592,9 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 
         if (beentry->st_state == STATE_RUNNING ||
             beentry->st_state == STATE_FASTPATH)
-            pgstat_count_conn_active_time(secs * 1000000 + usecs);
+            beentry->st_session_active_time += secs * 1000000 + usecs;
         else
-            pgstat_count_conn_txn_idle_time(secs * 1000000 + usecs);
+            beentry->st_session_idle_time += secs * 1000000 + usecs;
     }
 
     /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index f0e09eae4d..62b60077c1 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1635,6 +1635,56 @@ pg_stat_get_db_blk_write_time(PG_FUNCTION_ARGS)
     PG_RETURN_FLOAT8(result);
 }
 
+/* XXXXX */
+static double
+_pg_stat_active_session_stats(Oid dbid, int type)
+{
+    int            num_backends = pgstat_fetch_stat_numbackends();
+    int            curr_backend;
+    TimestampTz    now = GetCurrentTimestamp();
+    double        result = 0.0;
+    int            nsessions = 0;
+
+
+    /* Add session time of active backends */
+    /* 1-based index */
+    for (curr_backend = 1; curr_backend <= num_backends; curr_backend++)
+    {
+        LocalPgBackendStatus *local_beentry;
+        PgBackendStatus *beentry;
+
+        local_beentry = pgstat_fetch_stat_local_beentry(curr_backend);
+        if (!local_beentry)
+            continue;
+
+        beentry = &local_beentry->backendStatus;
+
+        if (beentry->st_databaseid != dbid)
+            continue;
+
+        switch (type) /* define enum !*/
+        {
+            case 0: /* SESSION TIME */
+                result +=
+                    (double)(now - beentry->st_proc_start_timestamp) / 1000;
+                break;
+            case 1: /* SESSION ACTIVE TIME */
+                result += (double)beentry->st_session_active_time / 1000;
+                break;
+            case 2: /* SESSION IDLE TIME */
+                result += (double)beentry->st_session_idle_time / 1000;
+                break;
+            case 3: /* SESSION NUMBER */
+                nsessions++;
+        }
+    }
+
+    if (type < 3)
+        return result;
+
+    return (double)nsessions;
+}
+
 Datum
 pg_stat_get_db_session_time(PG_FUNCTION_ARGS)
 {
@@ -1646,6 +1696,8 @@ pg_stat_get_db_session_time(PG_FUNCTION_ARGS)
     if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) != NULL)
         result = ((double) dbentry->total_session_time) / 1000.0;
 
+    result += _pg_stat_active_session_stats(dbid, 0);
+
     PG_RETURN_FLOAT8(result);
 }
 
@@ -1660,6 +1712,8 @@ pg_stat_get_db_active_time(PG_FUNCTION_ARGS)
     if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) != NULL)
         result = ((double) dbentry->total_active_time) / 1000.0;
 
+    result += _pg_stat_active_session_stats(dbid, 1);
+
     PG_RETURN_FLOAT8(result);
 }
 
@@ -1674,6 +1728,8 @@ pg_stat_get_db_idle_in_transaction_time(PG_FUNCTION_ARGS)
     if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) != NULL)
         result = ((double) dbentry->total_idle_in_xact_time) / 1000.0;
 
+    result += _pg_stat_active_session_stats(dbid, 2);
+
     PG_RETURN_FLOAT8(result);
 }
 
@@ -1687,6 +1743,7 @@ pg_stat_get_db_sessions(PG_FUNCTION_ARGS)
     if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) != NULL)
         result = (int64) (dbentry->n_sessions);
 
+    result += (int64)_pg_stat_active_session_stats(dbid, 3);
     PG_RETURN_INT64(result);
 }
 
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index f779b48b8c..9311111690 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -931,13 +931,6 @@ extern PgStat_MsgWal WalStats;
 extern PgStat_Counter pgStatBlockReadTime;
 extern PgStat_Counter pgStatBlockWriteTime;
 
-/*
- * Updated by pgstat_count_conn_*_time macros, called by
- * pgstat_report_activity().
- */
-extern PgStat_Counter pgStatActiveTime;
-extern PgStat_Counter pgStatTransactionIdleTime;
-
 
 /*
  * Updated by the traffic cop and in errfinish()
@@ -1039,10 +1032,6 @@ extern void pgstat_initstats(Relation rel);
     (pgStatBlockReadTime += (n))
 #define pgstat_count_buffer_write_time(n)                            \
     (pgStatBlockWriteTime += (n))
-#define pgstat_count_conn_active_time(n)                            \
-    (pgStatActiveTime += (n))
-#define pgstat_count_conn_txn_idle_time(n)                            \
-    (pgStatTransactionIdleTime += (n))
 
 extern void pgstat_count_heap_insert(Relation rel, PgStat_Counter n);
 extern void pgstat_count_heap_update(Relation rel, bool hot);
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 8042b817df..ebd5755247 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -124,6 +124,10 @@ typedef struct PgBackendStatus
     TimestampTz st_activity_start_timestamp;
     TimestampTz st_state_start_timestamp;
 
+    /* Session active/idle time in microsecnods */
+    int64        st_session_active_time;
+    int64        st_session_idle_time;
+
     /* Database OID, owning user's OID, connection client address */
     Oid            st_databaseid;
     Oid            st_userid;