Add a write_to_file member to PgStat_KindInfo
Started by Bertrand Drouvotabout 1 year ago12 messages
Hi hackers,
Working on [1]/messages/by-id/Zy4bmvgHqGjcK1pI@ip-10-97-1-34.eu-west-3.compute.internal produced the need to give to the statistics the ability to
decide whether or not they want to be written to the file on disk.
Indeed, there is no need to write the per backend I/O stats to disk (no point to
see stats for backends that do not exist anymore after a re-start), so $SUBJECT.
This new member could also be useful for custom statistics, so creating this
dedicated thread.
The attached patch also adds a test in the fixed injection points statistics.
It does not add one for the variable injection points statistics as I think adding
one test is enough (the code changes are simple enough).
[1]: /messages/by-id/Zy4bmvgHqGjcK1pI@ip-10-97-1-34.eu-west-3.compute.internal
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Add-a-write_to_file-member-to-PgStat_KindInfo.patchtext/x-diff; charset=us-asciiDownload
From 229cbce525382c36d461246dccc0ab6a71b31c17 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Wed, 20 Nov 2024 08:35:13 +0000
Subject: [PATCH v1] 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--1.0.sql | 13 ++
.../injection_points/injection_points.c | 6 +
.../injection_points/injection_stats.c | 1 +
.../injection_points/injection_stats.h | 6 +
.../injection_points/injection_stats_fixed.c | 149 ++++++++++++++++++
.../modules/injection_points/t/001_stats.pl | 14 ++
8 files changed, 211 insertions(+), 2 deletions(-)
9.7% src/backend/utils/activity/
11.1% src/test/modules/injection_points/t/
78.3% src/test/modules/injection_points/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index ea8c5691e8..4b8c57bb8e 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 stat kind allows 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))
@@ -1660,6 +1673,10 @@ pgstat_write_statsfile(XLogRecPtr redo)
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);
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_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index 6c81d55e0d..bcb9414a64 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -97,3 +97,16 @@ CREATE FUNCTION injection_points_stats_fixed(OUT numattach int8,
RETURNS record
AS 'MODULE_PATHNAME', 'injection_points_stats_fixed'
LANGUAGE C STRICT;
+
+--
+-- injection_points_stats_fixed_no_write()
+--
+-- Reports fixed-numbered (no write to file) statistics for injection points.
+CREATE FUNCTION injection_points_stats_fixed_no_write(OUT numattach int8,
+ OUT numdetach int8,
+ OUT numrun int8,
+ OUT numcached int8,
+ OUT numloaded int8)
+RETURNS record
+AS 'MODULE_PATHNAME', 'injection_points_stats_fixed_no_write'
+LANGUAGE C STRICT;
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 6bcde7b34e..0ab211abb5 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -358,6 +358,7 @@ injection_points_attach(PG_FUNCTION_ARGS)
}
pgstat_report_inj_fixed(1, 0, 0, 0, 0);
+ pgstat_report_inj_fixed_no_write(1, 0, 0, 0, 0);
InjectionPointAttach(name, "injection_points", function, &condition,
sizeof(InjectionPointCondition));
@@ -390,6 +391,7 @@ injection_points_load(PG_FUNCTION_ARGS)
injection_init_shmem();
pgstat_report_inj_fixed(0, 0, 0, 0, 1);
+ pgstat_report_inj_fixed_no_write(0, 0, 0, 0, 1);
INJECTION_POINT_LOAD(name);
PG_RETURN_VOID();
@@ -405,6 +407,7 @@ injection_points_run(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
pgstat_report_inj_fixed(0, 0, 1, 0, 0);
+ pgstat_report_inj_fixed_no_write(0, 0, 1, 0, 0);
INJECTION_POINT(name);
PG_RETURN_VOID();
@@ -420,6 +423,7 @@ injection_points_cached(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
pgstat_report_inj_fixed(0, 0, 0, 1, 0);
+ pgstat_report_inj_fixed_no_write(0, 0, 0, 1, 0);
INJECTION_POINT_CACHED(name);
PG_RETURN_VOID();
@@ -497,6 +501,7 @@ injection_points_detach(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
pgstat_report_inj_fixed(0, 1, 0, 0, 0);
+ pgstat_report_inj_fixed_no_write(0, 1, 0, 0, 0);
if (!InjectionPointDetach(name))
elog(ERROR, "could not detach injection point \"%s\"", name);
@@ -544,4 +549,5 @@ _PG_init(void)
pgstat_register_inj();
pgstat_register_inj_fixed();
+ pgstat_register_inj_fixed_no_write();
}
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.h b/src/test/modules/injection_points/injection_stats.h
index c48d533b4b..725f62f6e0 100644
--- a/src/test/modules/injection_points/injection_stats.h
+++ b/src/test/modules/injection_points/injection_stats.h
@@ -26,10 +26,16 @@ extern void pgstat_report_inj(const char *name);
/* injection_stats_fixed.c */
extern void pgstat_register_inj_fixed(void);
+extern void pgstat_register_inj_fixed_no_write(void);
extern void pgstat_report_inj_fixed(uint32 numattach,
uint32 numdetach,
uint32 numrun,
uint32 numcached,
uint32 numloaded);
+extern void pgstat_report_inj_fixed_no_write(uint32 numattach,
+ uint32 numdetach,
+ uint32 numrun,
+ uint32 numcached,
+ uint32 numloaded);
#endif
diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c
index 2fed178b7a..db36d07b67 100644
--- a/src/test/modules/injection_points/injection_stats_fixed.c
+++ b/src/test/modules/injection_points/injection_stats_fixed.c
@@ -44,12 +44,16 @@ typedef struct PgStatShared_InjectionPointFixed
/* Callbacks for fixed-numbered stats */
static void injection_stats_fixed_init_shmem_cb(void *stats);
+static void injection_stats_fixed_no_write_init_shmem_cb(void *stats);
static void injection_stats_fixed_reset_all_cb(TimestampTz ts);
+static void injection_stats_fixed_no_write_reset_all_cb(TimestampTz ts);
static void injection_stats_fixed_snapshot_cb(void);
+static void injection_stats_fixed_no_write_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),
@@ -60,10 +64,25 @@ static const PgStat_KindInfo injection_stats_fixed = {
.snapshot_cb = injection_stats_fixed_snapshot_cb,
};
+static const PgStat_KindInfo injection_stats_fixed_no_write = {
+ .name = "injection_points_fixed_no_write",
+ .fixed_amount = true,
+ .write_to_file = false,
+
+ .shared_size = sizeof(PgStat_StatInjFixedEntry),
+ .shared_data_off = offsetof(PgStatShared_InjectionPointFixed, stats),
+ .shared_data_len = sizeof(((PgStatShared_InjectionPointFixed *) 0)->stats),
+
+ .init_shmem_cb = injection_stats_fixed_no_write_init_shmem_cb,
+ .reset_all_cb = injection_stats_fixed_no_write_reset_all_cb,
+ .snapshot_cb = injection_stats_fixed_no_write_snapshot_cb,
+};
+
/*
* Kind ID reserved for statistics of injection points.
*/
#define PGSTAT_KIND_INJECTION_FIXED 130
+#define PGSTAT_KIND_INJECTION_FIXED_NO_WRITE 131
/* Track if fixed-numbered stats are loaded */
static bool inj_fixed_loaded = false;
@@ -77,6 +96,15 @@ injection_stats_fixed_init_shmem_cb(void *stats)
LWLockInitialize(&stats_shmem->lock, LWTRANCHE_PGSTATS_DATA);
}
+static void
+injection_stats_fixed_no_write_init_shmem_cb(void *stats)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ (PgStatShared_InjectionPointFixed *) stats;
+
+ LWLockInitialize(&stats_shmem->lock, LWTRANCHE_PGSTATS_DATA);
+}
+
static void
injection_stats_fixed_reset_all_cb(TimestampTz ts)
{
@@ -92,6 +120,21 @@ injection_stats_fixed_reset_all_cb(TimestampTz ts)
LWLockRelease(&stats_shmem->lock);
}
+static void
+injection_stats_fixed_no_write_reset_all_cb(TimestampTz ts)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+
+ LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
+ pgstat_copy_changecounted_stats(&stats_shmem->reset_offset,
+ &stats_shmem->stats,
+ sizeof(stats_shmem->stats),
+ &stats_shmem->changecount);
+ stats_shmem->stats.stat_reset_timestamp = ts;
+ LWLockRelease(&stats_shmem->lock);
+}
+
static void
injection_stats_fixed_snapshot_cb(void)
{
@@ -121,6 +164,35 @@ injection_stats_fixed_snapshot_cb(void)
#undef FIXED_COMP
}
+static void
+injection_stats_fixed_no_write_snapshot_cb(void)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ PgStat_StatInjFixedEntry *stat_snap =
+ pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ PgStat_StatInjFixedEntry *reset_offset = &stats_shmem->reset_offset;
+ PgStat_StatInjFixedEntry reset;
+
+ pgstat_copy_changecounted_stats(stat_snap,
+ &stats_shmem->stats,
+ sizeof(stats_shmem->stats),
+ &stats_shmem->changecount);
+
+ LWLockAcquire(&stats_shmem->lock, LW_SHARED);
+ memcpy(&reset, reset_offset, sizeof(stats_shmem->stats));
+ LWLockRelease(&stats_shmem->lock);
+
+ /* compensate by reset offsets */
+#define FIXED_COMP(fld) stat_snap->fld -= reset.fld;
+ FIXED_COMP(numattach);
+ FIXED_COMP(numdetach);
+ FIXED_COMP(numrun);
+ FIXED_COMP(numcached);
+ FIXED_COMP(numloaded);
+#undef FIXED_COMP
+}
+
/*
* Workhorse to do the registration work, called in _PG_init().
*/
@@ -133,6 +205,15 @@ pgstat_register_inj_fixed(void)
inj_fixed_loaded = true;
}
+void
+pgstat_register_inj_fixed_no_write(void)
+{
+ pgstat_register_kind(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE, &injection_stats_fixed_no_write);
+
+ /* mark stats as loaded */
+ inj_fixed_loaded = true;
+}
+
/*
* Report fixed number of statistics for an injection point.
*/
@@ -160,6 +241,30 @@ pgstat_report_inj_fixed(uint32 numattach,
pgstat_end_changecount_write(&stats_shmem->changecount);
}
+void
+pgstat_report_inj_fixed_no_write(uint32 numattach,
+ uint32 numdetach,
+ uint32 numrun,
+ uint32 numcached,
+ uint32 numloaded)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem;
+
+ /* leave if disabled */
+ if (!inj_fixed_loaded || !inj_stats_enabled)
+ return;
+
+ stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+
+ pgstat_begin_changecount_write(&stats_shmem->changecount);
+ stats_shmem->stats.numattach += numattach;
+ stats_shmem->stats.numdetach += numdetach;
+ stats_shmem->stats.numrun += numrun;
+ stats_shmem->stats.numcached += numcached;
+ stats_shmem->stats.numloaded += numloaded;
+ pgstat_end_changecount_write(&stats_shmem->changecount);
+}
+
/*
* SQL function returning fixed-numbered statistics for injection points.
*/
@@ -206,3 +311,47 @@ injection_points_stats_fixed(PG_FUNCTION_ARGS)
/* Returns the record as Datum */
PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
+
+PG_FUNCTION_INFO_V1(injection_points_stats_fixed_no_write);
+Datum
+injection_points_stats_fixed_no_write(PG_FUNCTION_ARGS)
+{
+ TupleDesc tupdesc;
+ Datum values[5] = {0};
+ bool nulls[5] = {0};
+ PgStat_StatInjFixedEntry *stats;
+
+ if (!inj_fixed_loaded || !inj_stats_enabled)
+ PG_RETURN_NULL();
+
+ pgstat_snapshot_fixed(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ stats = pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+
+ /* Initialise attributes information in the tuple descriptor */
+ tupdesc = CreateTemplateTupleDesc(5);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "numattach",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "numdetach",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "numrun",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 4, "numcached",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 5, "numloaded",
+ INT8OID, -1, 0);
+ BlessTupleDesc(tupdesc);
+
+ values[0] = Int64GetDatum(stats->numattach);
+ values[1] = Int64GetDatum(stats->numdetach);
+ values[2] = Int64GetDatum(stats->numrun);
+ values[3] = Int64GetDatum(stats->numcached);
+ values[4] = Int64GetDatum(stats->numloaded);
+ nulls[0] = false;
+ nulls[1] = false;
+ nulls[2] = false;
+ nulls[3] = false;
+ nulls[4] = false;
+
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
+}
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 36728f16fc..e5639029ee 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -39,6 +39,9 @@ is($numcalls, '2', 'number of stats calls');
my $fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls');
+my $fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed_no_write();");
+is($fixedstats_nowrite, '1|0|2|0|0', 'fixed stats (no write) after some calls');
# Loading and caching.
$node->safe_psql(
@@ -49,6 +52,10 @@ SELECT injection_points_cached('stats-notice');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|1|1', 'fixed stats after loading and caching');
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed_no_write();");
+is($fixedstats_nowrite, '1|0|2|1|1', 'fixed stats (no write) after loading and caching');
+
# Restart the node cleanly, stats should still be around.
$node->restart;
@@ -58,6 +65,10 @@ is($numcalls, '3', 'number of stats after clean restart');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|1|1', 'fixed stats after clean restart');
+# Except for the no write to file case
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed_no_write();");
+is($fixedstats_nowrite, '0|0|0|0|0', 'fixed stats (no write) after clean restart');
# On crash the stats are gone.
$node->stop('immediate');
@@ -68,6 +79,9 @@ is($numcalls, '', 'number of stats after crash');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed_no_write();");
+is($fixedstats_nowrite, '0|0|0|0|0', 'fixed stats (no write) after crash');
# Stop the server, disable the module, then restart. The server
# should be able to come up.
--
2.34.1
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
Thank you for working on this!
On Wed, 20 Nov 2024 at 17:05, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
Hi hackers,
Working on [1] produced the need to give to the statistics the ability to
decide whether or not they want to be written to the file on disk.Indeed, there is no need to write the per backend I/O stats to disk (no point to
see stats for backends that do not exist anymore after a re-start), so $SUBJECT.
I think this is a good idea. +1 for the $SUBJECT.
This new member could also be useful for custom statistics, so creating this
dedicated thread.The attached patch also adds a test in the fixed injection points statistics.
It does not add one for the variable injection points statistics as I think adding
one test is enough (the code changes are simple enough).
There are duplicated codes in the injection_stats_fixed.c file. Do you
think that 'modifying existing functions to take an argument to
differentiate whether the kind is default or no-write' would be
better?
--
Regards,
Nazir Bilal Yavuz
Microsoft
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
On Wed, Nov 20, 2024 at 05:45:55PM +0300, Nazir Bilal Yavuz wrote:
I think this is a good idea. +1 for the $SUBJECT.
Thanks for looking at it!
There are duplicated codes in the injection_stats_fixed.c file. Do you
think that 'modifying existing functions to take an argument to
differentiate whether the kind is default or no-write' would be
better?
Some of the new functions are callbacks so we don't have the control on the
parameters list that the core is expecting.
It remains:
pgstat_register_inj_fixed_no_write()
pgstat_report_inj_fixed_no_write()
injection_points_stats_fixed_no_write()
for which I think we could add an extra "write_to_file" argument on their original
counterparts.
Not sure how many code reduction we would get, so out of curiosity I did the
exercice in v2 attached and that gives us:
v1:
8 files changed, 211 insertions(+), 2 deletions(-)
v2:
8 files changed, 152 insertions(+), 22 deletions(-)
I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v2-0001-Add-a-write_to_file-member-to-PgStat_KindInfo.patchtext/x-diff; charset=us-asciiDownload
From 53f4afaed64d6975b022da22db7f5e7fe0134ca7 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Wed, 20 Nov 2024 08:35:13 +0000
Subject: [PATCH v2] 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--1.0.sql | 3 +-
.../injection_points/injection_points.c | 18 ++--
.../injection_points/injection_stats.c | 1 +
.../injection_points/injection_stats.h | 6 +-
.../injection_points/injection_stats_fixed.c | 100 ++++++++++++++++--
.../modules/injection_points/t/001_stats.pl | 22 +++-
8 files changed, 152 insertions(+), 22 deletions(-)
10.4% src/backend/utils/activity/
18.1% src/test/modules/injection_points/t/
70.5% src/test/modules/injection_points/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index ea8c5691e8..4b8c57bb8e 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 stat kind allows 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))
@@ -1660,6 +1673,10 @@ pgstat_write_statsfile(XLogRecPtr redo)
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);
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_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index 6c81d55e0d..67d5ab3d16 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -89,7 +89,8 @@ LANGUAGE C STRICT;
-- injection_points_stats_fixed()
--
-- Reports fixed-numbered statistics for injection points.
-CREATE FUNCTION injection_points_stats_fixed(OUT numattach int8,
+CREATE FUNCTION injection_points_stats_fixed(IN write_to_file bool,
+ OUT numattach int8,
OUT numdetach int8,
OUT numrun int8,
OUT numcached int8,
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 6bcde7b34e..2ed55f1580 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -357,7 +357,8 @@ injection_points_attach(PG_FUNCTION_ARGS)
condition.pid = MyProcPid;
}
- pgstat_report_inj_fixed(1, 0, 0, 0, 0);
+ pgstat_report_inj_fixed(1, 0, 0, 0, 0, true);
+ pgstat_report_inj_fixed(1, 0, 0, 0, 0, false);
InjectionPointAttach(name, "injection_points", function, &condition,
sizeof(InjectionPointCondition));
@@ -389,7 +390,8 @@ injection_points_load(PG_FUNCTION_ARGS)
if (inj_state == NULL)
injection_init_shmem();
- pgstat_report_inj_fixed(0, 0, 0, 0, 1);
+ pgstat_report_inj_fixed(0, 0, 0, 0, 1, true);
+ pgstat_report_inj_fixed(0, 0, 0, 0, 1, false);
INJECTION_POINT_LOAD(name);
PG_RETURN_VOID();
@@ -404,7 +406,8 @@ injection_points_run(PG_FUNCTION_ARGS)
{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
- pgstat_report_inj_fixed(0, 0, 1, 0, 0);
+ pgstat_report_inj_fixed(0, 0, 1, 0, 0, true);
+ pgstat_report_inj_fixed(0, 0, 1, 0, 0, false);
INJECTION_POINT(name);
PG_RETURN_VOID();
@@ -419,7 +422,8 @@ injection_points_cached(PG_FUNCTION_ARGS)
{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
- pgstat_report_inj_fixed(0, 0, 0, 1, 0);
+ pgstat_report_inj_fixed(0, 0, 0, 1, 0, true);
+ pgstat_report_inj_fixed(0, 0, 0, 1, 0, false);
INJECTION_POINT_CACHED(name);
PG_RETURN_VOID();
@@ -496,7 +500,8 @@ injection_points_detach(PG_FUNCTION_ARGS)
{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
- pgstat_report_inj_fixed(0, 1, 0, 0, 0);
+ pgstat_report_inj_fixed(0, 1, 0, 0, 0, true);
+ pgstat_report_inj_fixed(0, 1, 0, 0, 0, false);
if (!InjectionPointDetach(name))
elog(ERROR, "could not detach injection point \"%s\"", name);
@@ -543,5 +548,6 @@ _PG_init(void)
shmem_startup_hook = injection_shmem_startup;
pgstat_register_inj();
- pgstat_register_inj_fixed();
+ pgstat_register_inj_fixed(true);
+ pgstat_register_inj_fixed(false);
}
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.h b/src/test/modules/injection_points/injection_stats.h
index c48d533b4b..b42e141c85 100644
--- a/src/test/modules/injection_points/injection_stats.h
+++ b/src/test/modules/injection_points/injection_stats.h
@@ -25,11 +25,11 @@ extern void pgstat_drop_inj(const char *name);
extern void pgstat_report_inj(const char *name);
/* injection_stats_fixed.c */
-extern void pgstat_register_inj_fixed(void);
+extern void pgstat_register_inj_fixed(bool write_to_file);
extern void pgstat_report_inj_fixed(uint32 numattach,
uint32 numdetach,
uint32 numrun,
uint32 numcached,
- uint32 numloaded);
-
+ uint32 numloaded,
+ bool write_to_file);
#endif
diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c
index 2fed178b7a..862fa356ec 100644
--- a/src/test/modules/injection_points/injection_stats_fixed.c
+++ b/src/test/modules/injection_points/injection_stats_fixed.c
@@ -44,12 +44,16 @@ typedef struct PgStatShared_InjectionPointFixed
/* Callbacks for fixed-numbered stats */
static void injection_stats_fixed_init_shmem_cb(void *stats);
+static void injection_stats_fixed_no_write_init_shmem_cb(void *stats);
static void injection_stats_fixed_reset_all_cb(TimestampTz ts);
+static void injection_stats_fixed_no_write_reset_all_cb(TimestampTz ts);
static void injection_stats_fixed_snapshot_cb(void);
+static void injection_stats_fixed_no_write_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),
@@ -60,10 +64,25 @@ static const PgStat_KindInfo injection_stats_fixed = {
.snapshot_cb = injection_stats_fixed_snapshot_cb,
};
+static const PgStat_KindInfo injection_stats_fixed_no_write = {
+ .name = "injection_points_fixed_no_write",
+ .fixed_amount = true,
+ .write_to_file = false,
+
+ .shared_size = sizeof(PgStat_StatInjFixedEntry),
+ .shared_data_off = offsetof(PgStatShared_InjectionPointFixed, stats),
+ .shared_data_len = sizeof(((PgStatShared_InjectionPointFixed *) 0)->stats),
+
+ .init_shmem_cb = injection_stats_fixed_no_write_init_shmem_cb,
+ .reset_all_cb = injection_stats_fixed_no_write_reset_all_cb,
+ .snapshot_cb = injection_stats_fixed_no_write_snapshot_cb,
+};
+
/*
* Kind ID reserved for statistics of injection points.
*/
#define PGSTAT_KIND_INJECTION_FIXED 130
+#define PGSTAT_KIND_INJECTION_FIXED_NO_WRITE 131
/* Track if fixed-numbered stats are loaded */
static bool inj_fixed_loaded = false;
@@ -77,6 +96,15 @@ injection_stats_fixed_init_shmem_cb(void *stats)
LWLockInitialize(&stats_shmem->lock, LWTRANCHE_PGSTATS_DATA);
}
+static void
+injection_stats_fixed_no_write_init_shmem_cb(void *stats)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ (PgStatShared_InjectionPointFixed *) stats;
+
+ LWLockInitialize(&stats_shmem->lock, LWTRANCHE_PGSTATS_DATA);
+}
+
static void
injection_stats_fixed_reset_all_cb(TimestampTz ts)
{
@@ -92,6 +120,21 @@ injection_stats_fixed_reset_all_cb(TimestampTz ts)
LWLockRelease(&stats_shmem->lock);
}
+static void
+injection_stats_fixed_no_write_reset_all_cb(TimestampTz ts)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+
+ LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
+ pgstat_copy_changecounted_stats(&stats_shmem->reset_offset,
+ &stats_shmem->stats,
+ sizeof(stats_shmem->stats),
+ &stats_shmem->changecount);
+ stats_shmem->stats.stat_reset_timestamp = ts;
+ LWLockRelease(&stats_shmem->lock);
+}
+
static void
injection_stats_fixed_snapshot_cb(void)
{
@@ -121,13 +164,45 @@ injection_stats_fixed_snapshot_cb(void)
#undef FIXED_COMP
}
+static void
+injection_stats_fixed_no_write_snapshot_cb(void)
+{
+ PgStatShared_InjectionPointFixed *stats_shmem =
+ pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ PgStat_StatInjFixedEntry *stat_snap =
+ pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ PgStat_StatInjFixedEntry *reset_offset = &stats_shmem->reset_offset;
+ PgStat_StatInjFixedEntry reset;
+
+ pgstat_copy_changecounted_stats(stat_snap,
+ &stats_shmem->stats,
+ sizeof(stats_shmem->stats),
+ &stats_shmem->changecount);
+
+ LWLockAcquire(&stats_shmem->lock, LW_SHARED);
+ memcpy(&reset, reset_offset, sizeof(stats_shmem->stats));
+ LWLockRelease(&stats_shmem->lock);
+
+ /* compensate by reset offsets */
+#define FIXED_COMP(fld) stat_snap->fld -= reset.fld;
+ FIXED_COMP(numattach);
+ FIXED_COMP(numdetach);
+ FIXED_COMP(numrun);
+ FIXED_COMP(numcached);
+ FIXED_COMP(numloaded);
+#undef FIXED_COMP
+}
+
/*
* Workhorse to do the registration work, called in _PG_init().
*/
void
-pgstat_register_inj_fixed(void)
+pgstat_register_inj_fixed(bool write_to_file)
{
- pgstat_register_kind(PGSTAT_KIND_INJECTION_FIXED, &injection_stats_fixed);
+ if (write_to_file)
+ pgstat_register_kind(PGSTAT_KIND_INJECTION_FIXED, &injection_stats_fixed);
+ else
+ pgstat_register_kind(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE, &injection_stats_fixed_no_write);
/* mark stats as loaded */
inj_fixed_loaded = true;
@@ -141,7 +216,8 @@ pgstat_report_inj_fixed(uint32 numattach,
uint32 numdetach,
uint32 numrun,
uint32 numcached,
- uint32 numloaded)
+ uint32 numloaded,
+ bool write_to_file)
{
PgStatShared_InjectionPointFixed *stats_shmem;
@@ -149,7 +225,10 @@ pgstat_report_inj_fixed(uint32 numattach,
if (!inj_fixed_loaded || !inj_stats_enabled)
return;
- stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED);
+ if (write_to_file)
+ stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED);
+ else
+ stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
pgstat_begin_changecount_write(&stats_shmem->changecount);
stats_shmem->stats.numattach += numattach;
@@ -170,13 +249,22 @@ injection_points_stats_fixed(PG_FUNCTION_ARGS)
TupleDesc tupdesc;
Datum values[5] = {0};
bool nulls[5] = {0};
+ bool write_to_file = PG_GETARG_BOOL(0);
PgStat_StatInjFixedEntry *stats;
if (!inj_fixed_loaded || !inj_stats_enabled)
PG_RETURN_NULL();
- pgstat_snapshot_fixed(PGSTAT_KIND_INJECTION_FIXED);
- stats = pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED);
+ if (write_to_file)
+ {
+ pgstat_snapshot_fixed(PGSTAT_KIND_INJECTION_FIXED);
+ stats = pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED);
+ }
+ else
+ {
+ pgstat_snapshot_fixed(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ stats = pgstat_get_custom_snapshot_data(PGSTAT_KIND_INJECTION_FIXED_NO_WRITE);
+ }
/* Initialise attributes information in the tuple descriptor */
tupdesc = CreateTemplateTupleDesc(5);
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 36728f16fc..9ae329241c 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -37,8 +37,11 @@ my $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '2', 'number of stats calls');
my $fixedstats = $node->safe_psql('postgres',
- "SELECT * FROM injection_points_stats_fixed();");
+ "SELECT * FROM injection_points_stats_fixed(true);");
is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls');
+my $fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed(false);");
+is($fixedstats_nowrite, '1|0|2|0|0', 'fixed stats (no write) after some calls');
# Loading and caching.
$node->safe_psql(
@@ -47,8 +50,12 @@ SELECT injection_points_load('stats-notice');
SELECT injection_points_cached('stats-notice');
");
$fixedstats = $node->safe_psql('postgres',
- "SELECT * FROM injection_points_stats_fixed();");
+ "SELECT * FROM injection_points_stats_fixed(true);");
is($fixedstats, '1|0|2|1|1', 'fixed stats after loading and caching');
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed(false);");
+is($fixedstats_nowrite, '1|0|2|1|1', 'fixed stats (no write) after loading and caching');
+
# Restart the node cleanly, stats should still be around.
$node->restart;
@@ -56,8 +63,12 @@ $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '3', 'number of stats after clean restart');
$fixedstats = $node->safe_psql('postgres',
- "SELECT * FROM injection_points_stats_fixed();");
+ "SELECT * FROM injection_points_stats_fixed(true);");
is($fixedstats, '1|0|2|1|1', 'fixed stats after clean restart');
+# Except for the no write to file case
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed(false);");
+is($fixedstats_nowrite, '0|0|0|0|0', 'fixed stats (no write) after clean restart');
# On crash the stats are gone.
$node->stop('immediate');
@@ -66,8 +77,11 @@ $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'number of stats after crash');
$fixedstats = $node->safe_psql('postgres',
- "SELECT * FROM injection_points_stats_fixed();");
+ "SELECT * FROM injection_points_stats_fixed(true);");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
+$fixedstats_nowrite = $node->safe_psql('postgres',
+ "SELECT * FROM injection_points_stats_fixed(false);");
+is($fixedstats_nowrite, '0|0|0|0|0', 'fixed stats (no write) after crash');
# Stop the server, disable the module, then restart. The server
# should be able to come up.
--
2.34.1
Re: Add a write_to_file member to PgStat_KindInfo
On Wed, Nov 20, 2024 at 05:13:18PM +0000, Bertrand Drouvot wrote:
I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.
Well, I like the enthusiasm of having tests, but injection_points
stats being persistent on disk is the only write property I want to
keep in the module, because it can be useful across clean restarts.
Your patch results in a weird mix where you now need to have a total
of four stats IDs rather than two: one more for the fixed-numbered
case and one more for the variable-numbered case to be able to control
the write/no-write property set in shared memory. The correct design,
if we were to control the write/no-write for individual entries, would
be to store a state flag in each individual entries and decide if an
entry should be flushed or not, which would require an additional
callback to check the state of an individual entry at write. Not sure
if we really need that, TBH, until someone comes up with a case for
it.
Your patch adding backend statistics would be a much better fit to add
a test for this specific property, so let's keep injection_points out
of it, even if it means that we would have only coverage for
variable-numbered stats. So let's keep it simple here, and just set
.write_to_file to true for both injection_points stats kinds per the
scope of this thread. Another point is that injection_points acts as
a template for custom pgstats, this makes the code harder to use as a
reference.
Point taken that the tests added in v2 show that the patch works.
--
Michael
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
On Thu, Nov 21, 2024 at 10:01:07AM +0900, Michael Paquier wrote:
On Wed, Nov 20, 2024 at 05:13:18PM +0000, Bertrand Drouvot wrote:
I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.Your patch adding backend statistics would be a much better fit to add
a test for this specific property
Right.
so let's keep injection_points out
of it, even if it means that we would have only coverage for
variable-numbered stats.
Right, and only when per-backend I/O stats goes in.
So let's keep it simple here, and just set
.write_to_file to true for both injection_points stats kinds per the
scope of this thread.
That's fine by me (the code is simple enough anyway). Done that way in v3
attached.
Another point is that injection_points acts as
a template for custom pgstats
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.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Add-a-write_to_file-member-to-PgStat_KindInfo.patchtext/x-diff; charset=us-asciiDownload
From 86272b17ecb58c04931eb268d20714a7b389c3af Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Wed, 20 Nov 2024 08:35:13 +0000
Subject: [PATCH v3] 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.1% 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..4b8c57bb8e 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 stat kind allows 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))
@@ -1660,6 +1673,10 @@ pgstat_write_statsfile(XLogRecPtr redo)
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);
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
Re: Add a write_to_file member to PgStat_KindInfo
On Thu, Nov 21, 2024 at 06:32:03AM +0000, Bertrand Drouvot 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.
Plus/minus some tweaks in the wordings (like a s/stat /stats /), the
logic is sound, and that's a lot simpler.
--
Michael
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
On Thu, 21 Nov 2024 at 09:32, Bertrand Drouvot
<bertranddrouvot.pg@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);
=== 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);
===--
Regards,
Nazir Bilal Yavuz
Microsoft
Re: Add a write_to_file member to PgStat_KindInfo
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.pg@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
Attachments:
v4-0001-Add-a-write_to_file-member-to-PgStat_KindInfo.patchtext/x-diff; charset=us-asciiDownload
From 37009b778e83a1f4d6d5365d0ab90db2daa62176 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@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
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
On Thu, Nov 21, 2024 at 04:26:47PM +0900, Michael Paquier wrote:
On Thu, Nov 21, 2024 at 06:32:03AM +0000, Bertrand Drouvot 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.Plus/minus some tweaks in the wordings (like a s/stat /stats /),
I think the singular was ok here, but v4 (just shared up-thread) uses your
proposal.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: Add a write_to_file member to PgStat_KindInfo
Hi,
Thanks for patch. I have tested both patches and they work as per design.
My +1 for v1, it is much cleaner approach and has properly named
functions whereas in v2 it is not.
Injection points is documented, if so, doc patch is missing.
Regards,
Yogesh
Show quoted text
On 11/20/24 12:13, Bertrand Drouvot wrote:
Hi,
On Wed, Nov 20, 2024 at 05:45:55PM +0300, Nazir Bilal Yavuz wrote:
I think this is a good idea. +1 for the $SUBJECT.
Thanks for looking at it!
There are duplicated codes in the injection_stats_fixed.c file. Do you
think that 'modifying existing functions to take an argument to
differentiate whether the kind is default or no-write' would be
better?Some of the new functions are callbacks so we don't have the control on the
parameters list that the core is expecting.It remains:
pgstat_register_inj_fixed_no_write()
pgstat_report_inj_fixed_no_write()
injection_points_stats_fixed_no_write()for which I think we could add an extra "write_to_file" argument on their original
counterparts.Not sure how many code reduction we would get, so out of curiosity I did the
exercice in v2 attached and that gives us:v1:
8 files changed, 211 insertions(+), 2 deletions(-)v2:
8 files changed, 152 insertions(+), 22 deletions(-)I don't have a strong opinion for this particular case here (I think the code
is harder to read but yeah there is some code reduction): so I'm fine with
v2 too.Regards,
Re: Add a write_to_file member to PgStat_KindInfo
On Thu, Nov 21, 2024 at 08:49:13AM +0000, Bertrand Drouvot wrote:
On Thu, Nov 21, 2024 at 10:38:28AM +0300, Nazir Bilal Yavuz wrote:
- 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.
It could still be possible that a builtin fixed-numbered stats kind is
introduced with !write_to_file. I'd want the sanity check on
snapshot_ctl_off also in this case. As-is, you are right that it does
not really matter as we use this field only to grab the fixed data
from the stats snapshot before writing it to the file, but having this
sanity check may avoid issues in the long run should write_to_file be
flipped from false to true for some reason, and the check is a free
bonus.
=== 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.
Right. Checking the refcount is a good thing even if we don't write
such stats entries.
I've done both of these things, and applied the patch. Let's move on
to the next things.
--
Michael