Add callbacks to control flush of fixed-numbered stats

This commit adds two callbacks in pgstats to have a better control of
the flush timing of pgstat_report_stat(), whose operation depends on the
three PGSTAT_*_INTERVAL variables:
- have_fixed_pending_cb(), to check if a stats kind has any pending
data waiting for a flush.  This is used as a fast path if there are no
pending statistics to flush, and this check is done for fixed-numbered
statistics only if there are no variable-numbered statistics to flush.
A flush will need to happen if at least one callback reports any pending
data.
- flush_fixed_cb(), to do the actual flush.

These callbacks are currently used by the SLRU, WAL and IO statistics,
generalizing the concept for all stats kinds (builtin and custom).

The SLRU and IO stats relied each on one global variable to determine
whether a flush should happen; these are now local to pgstat_slru.c and
pgstat_io.c, cleaning up a bit how the pending flush states are tracked
in pgstat.c.

pgstat_flush_io() and pgstat_flush_wal() are still required, but we do
not need to check their return result anymore.

Reviewed-by: Bertrand Drouvot, Kyotaro Horiguchi
Discussion: https://postgr.es/m/ZtaVO0N-aTwiAk3w@paquier.xyz

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 178b5ef65aab4939aabd4cc7115c8c2d3640c964..a7f2dfc744cfca68bdcdde13f639108b3a0a91d0 100644 (file)
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -411,6 +411,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
        .shared_data_off = offsetof(PgStatShared_IO, stats),
        .shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats),
 
+       .flush_fixed_cb = pgstat_io_flush_cb,
+       .have_fixed_pending_cb = pgstat_io_have_pending_cb,
        .init_shmem_cb = pgstat_io_init_shmem_cb,
        .reset_all_cb = pgstat_io_reset_all_cb,
        .snapshot_cb = pgstat_io_snapshot_cb,
@@ -426,6 +428,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
        .shared_data_off = offsetof(PgStatShared_SLRU, stats),
        .shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
 
+       .flush_fixed_cb = pgstat_slru_flush_cb,
+       .have_fixed_pending_cb = pgstat_slru_have_pending_cb,
        .init_shmem_cb = pgstat_slru_init_shmem_cb,
        .reset_all_cb = pgstat_slru_reset_all_cb,
        .snapshot_cb = pgstat_slru_snapshot_cb,
@@ -442,6 +446,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
        .shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
 
        .init_backend_cb = pgstat_wal_init_backend_cb,
+       .flush_fixed_cb = pgstat_wal_flush_cb,
+       .have_fixed_pending_cb = pgstat_wal_have_pending_cb,
        .init_shmem_cb = pgstat_wal_init_shmem_cb,
        .reset_all_cb = pgstat_wal_reset_all_cb,
        .snapshot_cb = pgstat_wal_snapshot_cb,
@@ -671,13 +677,37 @@ pgstat_report_stat(bool force)
    }
 
    /* Don't expend a clock check if nothing to do */
-   if (dlist_is_empty(&pgStatPending) &&
-       !have_iostats &&
-       !have_slrustats &&
-       !pgstat_have_pending_wal())
+   if (dlist_is_empty(&pgStatPending))
    {
-       Assert(pending_since == 0);
-       return 0;
+       bool        do_flush = false;
+
+       /* Check for pending fixed-numbered stats */
+       for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+       {
+           const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+           if (!kind_info)
+               continue;
+           if (!kind_info->fixed_amount)
+           {
+               Assert(kind_info->have_fixed_pending_cb == NULL);
+               continue;
+           }
+           if (!kind_info->have_fixed_pending_cb)
+               continue;
+
+           if (kind_info->have_fixed_pending_cb())
+           {
+               do_flush = true;
+               break;
+           }
+       }
+
+       if (!do_flush)
+       {
+           Assert(pending_since == 0);
+           return 0;
+       }
    }
 
    /*
@@ -730,14 +760,23 @@ pgstat_report_stat(bool force)
    /* flush database / relation / function / ... stats */
    partial_flush |= pgstat_flush_pending_entries(nowait);
 
-   /* flush IO stats */
-   partial_flush |= pgstat_flush_io(nowait);
+   /* flush of fixed-numbered stats */
+   for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+   {
+       const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
 
-   /* flush wal stats */
-   partial_flush |= pgstat_flush_wal(nowait);
+       if (!kind_info)
+           continue;
+       if (!kind_info->fixed_amount)
+       {
+           Assert(kind_info->flush_fixed_cb == NULL);
+           continue;
+       }
+       if (!kind_info->flush_fixed_cb)
+           continue;
 
-   /* flush SLRU stats */
-   partial_flush |= pgstat_slru_flush(nowait);
+       partial_flush |= kind_info->flush_fixed_cb(nowait);
+   }
 
    last_flush = now;
 

diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 8af55989eed83da95d12e43187009093305c5018..cc2ffc78aa92fadd8dd34d4f0a8473e2bad228f3 100644 (file)
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -29,7 +29,7 @@ typedef struct PgStat_PendingIO
 
 
 static PgStat_PendingIO PendingIOStats;
-bool       have_iostats = false;
+static bool have_iostats = false;
 
 
 /*
@@ -161,6 +161,24 @@ pgstat_fetch_stat_io(void)
    return &pgStatLocal.snapshot.io;
 }
 
+/*
+ * Check if there any IO stats waiting for flush.
+ */
+bool
+pgstat_io_have_pending_cb(void)
+{
+   return have_iostats;
+}
+
+/*
+ * Simpler wrapper of pgstat_io_flush_cb()
+ */
+void
+pgstat_flush_io(bool nowait)
+{
+   (void) pgstat_io_flush_cb(nowait);
+}
+
 /*
  * Flush out locally pending IO statistics
  *
@@ -170,7 +188,7 @@ pgstat_fetch_stat_io(void)
  * acquired. Otherwise, return false.
  */
 bool
-pgstat_flush_io(bool nowait)
+pgstat_io_flush_cb(bool nowait)
 {
    LWLock     *bktype_lock;
    PgStat_BktypeIO *bktype_shstats;

diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c
index 6f922a85bfc690c3ef50816b38daf874efa701c3..dd6f9f840e73fddcb774652b6aed9be7b581815b 100644 (file)
--- a/src/backend/utils/activity/pgstat_slru.c
+++ b/src/backend/utils/activity/pgstat_slru.c
@@ -32,7 +32,7 @@ static void pgstat_reset_slru_counter_internal(int index, TimestampTz ts);
  * in order to avoid memory allocation.
  */
 static PgStat_SLRUStats pending_SLRUStats[SLRU_NUM_ELEMENTS];
-bool       have_slrustats = false;
+static bool have_slrustats = false;
 
 
 /*
@@ -143,6 +143,15 @@ pgstat_get_slru_index(const char *name)
    return (SLRU_NUM_ELEMENTS - 1);
 }
 
+/*
+ * Check if there are any SLRU stats entries waiting for flush.
+ */
+bool
+pgstat_slru_have_pending_cb(void)
+{
+   return have_slrustats;
+}
+
 /*
  * Flush out locally pending SLRU stats entries
  *
@@ -153,7 +162,7 @@ pgstat_get_slru_index(const char *name)
  * acquired. Otherwise return false.
  */
 bool
-pgstat_slru_flush(bool nowait)
+pgstat_slru_flush_cb(bool nowait)
 {
    PgStatShared_SLRU *stats_shmem = &pgStatLocal.shmem->slru;
    int         i;

diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index 8c19c3f2fd52d1d80bbc7423148ad3e55f9347ce..e1d371ba3cd18e34e10f94d115dcc2596865d07e 100644 (file)
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -71,6 +71,15 @@ pgstat_fetch_stat_wal(void)
    return &pgStatLocal.snapshot.wal;
 }
 
+/*
+ * Simple wrapper of pgstat_wal_flush_cb()
+ */
+void
+pgstat_flush_wal(bool nowait)
+{
+   (void) pgstat_wal_flush_cb(nowait);
+}
+
 /*
  * Calculate how much WAL usage counters have increased by subtracting the
  * previous counters from the current ones.
@@ -79,7 +88,7 @@ pgstat_fetch_stat_wal(void)
  * acquired. Otherwise return false.
  */
 bool
-pgstat_flush_wal(bool nowait)
+pgstat_wal_flush_cb(bool nowait)
 {
    PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
    WalUsage    wal_usage_diff = {0};
@@ -92,7 +101,7 @@ pgstat_flush_wal(bool nowait)
     * This function can be called even if nothing at all has happened. Avoid
     * taking lock for nothing in that case.
     */
-   if (!pgstat_have_pending_wal())
+   if (!pgstat_wal_have_pending_cb())
        return false;
 
    /*
@@ -141,8 +150,8 @@ void
 pgstat_wal_init_backend_cb(void)
 {
    /*
-    * Initialize prevWalUsage with pgWalUsage so that pgstat_flush_wal() can
-    * calculate how much pgWalUsage counters are increased by subtracting
+    * Initialize prevWalUsage with pgWalUsage so that pgstat_wal_flush_cb()
+    * can calculate how much pgWalUsage counters are increased by subtracting
     * prevWalUsage from pgWalUsage.
     */
    prevWalUsage = pgWalUsage;
@@ -156,7 +165,7 @@ pgstat_wal_init_backend_cb(void)
  * data pages.
  */
 bool
-pgstat_have_pending_wal(void)
+pgstat_wal_have_pending_cb(void)
 {
    return pgWalUsage.wal_records != prevWalUsage.wal_records ||
        PendingWalStats.wal_write != 0 ||

diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 25820cbf0a6b41e920c19af620dfee8a5cc512e2..bba90e898dd353e5b1ac550085a5636034cb7b7b 100644 (file)
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -237,7 +237,7 @@ typedef struct PgStat_KindInfo
 
    /*
     * For variable-numbered stats: flush pending stats. Required if pending
-    * data is used.
+    * data is used.  See flush_fixed_cb for fixed-numbered stats.
     */
    bool        (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
 
@@ -265,6 +265,19 @@ typedef struct PgStat_KindInfo
     */
    void        (*init_shmem_cb) (void *stats);
 
+   /*
+    * For fixed-numbered statistics: Flush pending stats. Returns true if
+    * some of the stats could not be flushed, due to lock contention for
+    * example. Optional.
+    */
+   bool        (*flush_fixed_cb) (bool nowait);
+
+   /*
+    * For fixed-numbered statistics: Check for pending stats in need of
+    * flush. Returns true if there are any stats pending for flush. Optional.
+    */
+   bool        (*have_fixed_pending_cb) (void);
+
    /*
     * For fixed-numbered statistics: Reset All.
     */
@@ -609,7 +622,10 @@ extern bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
  * Functions in pgstat_io.c
  */
 
-extern bool pgstat_flush_io(bool nowait);
+extern void pgstat_flush_io(bool nowait);
+
+extern bool pgstat_io_have_pending_cb(void);
+extern bool pgstat_io_flush_cb(bool nowait);
 extern void pgstat_io_init_shmem_cb(void *stats);
 extern void pgstat_io_reset_all_cb(TimestampTz ts);
 extern void pgstat_io_snapshot_cb(void);
@@ -668,7 +684,8 @@ extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
  * Functions in pgstat_slru.c
  */
 
-extern bool pgstat_slru_flush(bool nowait);
+extern bool pgstat_slru_have_pending_cb(void);
+extern bool pgstat_slru_flush_cb(bool nowait);
 extern void pgstat_slru_init_shmem_cb(void *stats);
 extern void pgstat_slru_reset_all_cb(TimestampTz ts);
 extern void pgstat_slru_snapshot_cb(void);
@@ -678,10 +695,11 @@ extern void pgstat_slru_snapshot_cb(void);
  * Functions in pgstat_wal.c
  */
 
-extern bool pgstat_flush_wal(bool nowait);
-extern bool pgstat_have_pending_wal(void);
+extern void pgstat_flush_wal(bool nowait);
 
 extern void pgstat_wal_init_backend_cb(void);
+extern bool pgstat_wal_have_pending_cb(void);
+extern bool pgstat_wal_flush_cb(bool nowait);
 extern void pgstat_wal_init_shmem_cb(void *stats);
 extern void pgstat_wal_reset_all_cb(TimestampTz ts);
 extern void pgstat_wal_snapshot_cb(void);
@@ -711,20 +729,6 @@ extern void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid)
 extern PGDLLIMPORT PgStat_LocalState pgStatLocal;
 
 
-/*
- * Variables in pgstat_io.c
- */
-
-extern PGDLLIMPORT bool have_iostats;
-
-
-/*
- * Variables in pgstat_slru.c
- */
-
-extern PGDLLIMPORT bool have_slrustats;
-
-
 /*
  * Implementation of inline functions declared above.
  */