Hi,
On Thu, Nov 21, 2024 at 10:38:28AM +0300, Nazir Bilal Yavuz wrote:
> Hi,
>
> On Thu, 21 Nov 2024 at 09:32, Bertrand Drouvot
> <bertranddrouvot...@gmail.com> wrote:
> > That was in fact the main reason why I added this test. But well, just
> > adding the "write_to_file" in the injection test is enough to "show" that
> > this
> > member does exist. So I'm fine with v3.
>
> I think that the changes below (write_to_file checks) should come
> after the assertion checks. Otherwise, they could mask some problems.
>
> === 1
> - if (!info || !info->fixed_amount)
> + /* skip if not fixed or this kind does not want to write to the file
> */
> + if (!info || !info->fixed_amount || !info->write_to_file)
> continue;
>
> if (pgstat_is_kind_builtin(kind))
> Assert(info->snapshot_ctl_off != 0);
We need "if (!info || !info->fixed_amount)" before the assertion check (as
it makes sense only for fixed stats). Then I'm not sure to create a new branch
for the "write_to_file" check after this assertion is worth it.
> === 2
> kind_info = pgstat_get_kind_info(ps->key.kind);
>
> + /* skip if this kind does not want to write to the file */
> + if (!kind_info->write_to_file)
> + continue;
> +
> /* if not dropped the valid-entry refcount should exist */
> Assert(pg_atomic_read_u32(&ps->refcount) > 0);
Makes sense, done in v4 attached.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 37009b778e83a1f4d6d5365d0ab90db2daa62176 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Wed, 20 Nov 2024 08:35:13 +0000
Subject: [PATCH v4] Add a write_to_file member to PgStat_KindInfo
This new member allows the statistics to configure whether or not they want to
be written to the file on disk. That gives more flexiblity to the custom
statistics.
---
src/backend/utils/activity/pgstat.c | 21 +++++++++++++++++--
src/include/utils/pgstat_internal.h | 3 +++
.../injection_points/injection_stats.c | 1 +
.../injection_points/injection_stats_fixed.c | 1 +
4 files changed, 24 insertions(+), 2 deletions(-)
87.2% src/backend/utils/activity/
6.9% src/include/utils/
5.8% src/test/modules/injection_points/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index ea8c5691e8..01f82279bf 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -12,7 +12,8 @@
* Statistics are loaded from the filesystem during startup (by the startup
* process), unless preceded by a crash, in which case all stats are
* discarded. They are written out by the checkpointer process just before
- * shutting down, except when shutting down in immediate mode.
+ * shutting down (if the statistics kind allow it), except when shutting down in
+ * immediate mode.
*
* Fixed-numbered stats are stored in plain (non-dynamic) shared memory.
*
@@ -281,6 +282,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "database",
.fixed_amount = false,
+ .write_to_file = true,
/* so pg_stat_database entries can be seen in all databases */
.accessed_across_databases = true,
@@ -297,6 +299,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "relation",
.fixed_amount = false,
+ .write_to_file = true,
.shared_size = sizeof(PgStatShared_Relation),
.shared_data_off = offsetof(PgStatShared_Relation, stats),
@@ -311,6 +314,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "function",
.fixed_amount = false,
+ .write_to_file = true,
.shared_size = sizeof(PgStatShared_Function),
.shared_data_off = offsetof(PgStatShared_Function, stats),
@@ -324,6 +328,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "replslot",
.fixed_amount = false,
+ .write_to_file = true,
.accessed_across_databases = true,
@@ -340,6 +345,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "subscription",
.fixed_amount = false,
+ .write_to_file = true,
/* so pg_stat_subscription_stats entries can be seen in all databases */
.accessed_across_databases = true,
@@ -359,6 +365,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "archiver",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, archiver),
.shared_ctl_off = offsetof(PgStat_ShmemControl, archiver),
@@ -374,6 +381,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "bgwriter",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, bgwriter),
.shared_ctl_off = offsetof(PgStat_ShmemControl, bgwriter),
@@ -389,6 +397,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "checkpointer",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, checkpointer),
.shared_ctl_off = offsetof(PgStat_ShmemControl, checkpointer),
@@ -404,6 +413,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "io",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, io),
.shared_ctl_off = offsetof(PgStat_ShmemControl, io),
@@ -421,6 +431,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "slru",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, slru),
.shared_ctl_off = offsetof(PgStat_ShmemControl, slru),
@@ -438,6 +449,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.name = "wal",
.fixed_amount = true,
+ .write_to_file = true,
.snapshot_ctl_off = offsetof(PgStat_Snapshot, wal),
.shared_ctl_off = offsetof(PgStat_ShmemControl, wal),
@@ -1611,7 +1623,8 @@ pgstat_write_statsfile(XLogRecPtr redo)
char *ptr;
const PgStat_KindInfo *info = pgstat_get_kind_info(kind);
- if (!info || !info->fixed_amount)
+ /* skip if not fixed or this kind does not want to write to the file */
+ if (!info || !info->fixed_amount || !info->write_to_file)
continue;
if (pgstat_is_kind_builtin(kind))
@@ -1663,6 +1676,10 @@ pgstat_write_statsfile(XLogRecPtr redo)
/* if not dropped the valid-entry refcount should exist */
Assert(pg_atomic_read_u32(&ps->refcount) > 0);
+ /* skip if this kind does not want to write to the file */
+ if (!kind_info->write_to_file)
+ continue;
+
if (!kind_info->to_serialized_name)
{
/* normal stats entry, identified by PgStat_HashKey */
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 437db06910..57ac6e87b6 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -213,6 +213,9 @@ typedef struct PgStat_KindInfo
*/
bool accessed_across_databases:1;
+ /* Write or not to the file */
+ bool write_to_file:1;
+
/*
* The size of an entry in the shared stats hash table (pointed to by
* PgStatShared_HashEntry->body). For fixed-numbered statistics, this is
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index d89d055913..e16b9db284 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -39,6 +39,7 @@ static bool injection_stats_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
static const PgStat_KindInfo injection_stats = {
.name = "injection_points",
.fixed_amount = false, /* Bounded by the number of points */
+ .write_to_file = true,
/* Injection points are system-wide */
.accessed_across_databases = true,
diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c
index 2fed178b7a..c5b0bb8cf0 100644
--- a/src/test/modules/injection_points/injection_stats_fixed.c
+++ b/src/test/modules/injection_points/injection_stats_fixed.c
@@ -50,6 +50,7 @@ static void injection_stats_fixed_snapshot_cb(void);
static const PgStat_KindInfo injection_stats_fixed = {
.name = "injection_points_fixed",
.fixed_amount = true,
+ .write_to_file = true,
.shared_size = sizeof(PgStat_StatInjFixedEntry),
.shared_data_off = offsetof(PgStatShared_InjectionPointFixed, stats),
--
2.34.1
