Add support for entry counting in pgstats
Hi all,
One obstacle to the move of pg_stat_statements to the shmem pgstats
that we have in core is that there is no easy way to track the total
number of entries that are stored in the shared hash table of pgstats
for variable-sized stats kinds.
PGSS currently hash_get_num_entries() as a cheap way to get the number
of entries stored in its hash table, but moving PGSS to the in-core
stats facility means that we need a different approach to count these
entries.
I have looked at what can be done, and finished with a rather simple
approach, as we have only one code path when a new entry is inserted,
and one code path when an entry is removed when the refcount of an
entry reaches 0 (includes resets, drops for kind matches, etc.). The
counters are stored in a uint64 atomic, one for each stats kind in
PgStat_ShmemControl, set only if the option is enabled.
I am wondering how much protection we want for the reads, and chose
the lightest "lossy" approach, leaving to stats kind what they want to
do when deallocating entries. Hence, a stats kind may want to protect
the entry deallocations with an extra lock, but that's not required
either depending on how aggressive removals should happen.
Please find attached that adds an option for variable-sized stats
kinds given these the possibility to track the number of entries in
the shared hash table. The option is named track_counts. The patch
has some coverage added in the test module injection_points, in its
TAP test.
Thoughts are welcome.
--
Michael
Attachments:
0001-Add-support-for-entry-counting-in-pgstats.patchtext/x-diff; charset=us-asciiDownload
From 0c924a72f586c385f6ab1a22174b9c3b1cf2dd08 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 12 Sep 2025 16:09:51 +0900
Subject: [PATCH 1/2] Add support for entry counting in pgstats
Stats kinds can set an option call track_counts, that will make pgstats
track the number of entries that exist in the shared hashtable.
As there is only one code path where a new entry is added, and one code
path where entries are removed, the count tracking is rather
straight-forward. Reads of these counters are optimistic, and may
change across two calls.
A use case of this facility is PGSS, where we need to be able to cap the
number of entries that would be stored in the shared hashtable, based on
its "max" GUC.
---
src/include/utils/pgstat_internal.h | 26 +++++++++++++
src/backend/utils/activity/pgstat.c | 6 +++
src/backend/utils/activity/pgstat_shmem.c | 47 +++++++++++++++++------
3 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 6cf00008f633..8762a06081f8 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -216,6 +216,12 @@ typedef struct PgStat_KindInfo
/* Should stats be written to the on-disk stats file? */
bool write_to_file:1;
+ /*
+ * Should the number of entries be tracked? For variable-numbered stats,
+ * to update its PgStat_ShmemControl.entry_counts.
+ */
+ bool track_counts:1;
+
/*
* The size of an entry in the shared stats hash table (pointed to by
* PgStatShared_HashEntry->body). For fixed-numbered statistics, this is
@@ -485,6 +491,15 @@ typedef struct PgStat_ShmemControl
*/
pg_atomic_uint64 gc_request_count;
+ /*
+ * Counters for the number of entries associated to a single stats kind
+ * that uses variable-numbered objects stored in the shared hash table.
+ * These counters can be enabled on a per-kind basis, when track_counts is
+ * set. This counter is incremented each time a new entry is created (not
+ * when reused), and each time an entry is dropped.
+ */
+ pg_atomic_uint64 entry_counts[PGSTAT_KIND_MAX];
+
/*
* Stats data for fixed-numbered objects.
*/
@@ -910,6 +925,17 @@ pgstat_get_entry_data(PgStat_Kind kind, PgStatShared_Common *entry)
return ((char *) (entry)) + off;
}
+/*
+ * Returns the number of entries counted for a stats kind.
+ */
+static inline uint64
+pgstat_get_entry_count(PgStat_Kind kind)
+{
+ Assert(pgstat_get_kind_info(kind)->track_counts);
+
+ return pg_atomic_read_u64(&pgStatLocal.shmem->entry_counts[kind - 1]);
+}
+
/*
* Returns a pointer to the shared memory area of custom stats for
* fixed-numbered statistics.
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index f8e91484e36b..f6784ad6cd15 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1490,6 +1490,12 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info)
ereport(ERROR,
(errmsg("custom cumulative statistics property is invalid"),
errhint("Custom cumulative statistics require a shared memory size for fixed-numbered objects.")));
+ if (kind_info->track_counts)
+ {
+ ereport(ERROR,
+ (errmsg("custom cumulative statistics property is invalid"),
+ errhint("Custom cumulative statistics cannot use counter tracking for fixed-numbered objects.")));
+ }
}
/*
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 9dc3212f7dd0..141ab10031cd 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -210,27 +210,35 @@ StatsShmemInit(void)
pg_atomic_init_u64(&ctl->gc_request_count, 1);
- /* initialize fixed-numbered stats */
+ /* Do the per-kind initialization */
for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
char *ptr;
- if (!kind_info || !kind_info->fixed_amount)
+ if (!kind_info)
continue;
- if (pgstat_is_kind_builtin(kind))
- ptr = ((char *) ctl) + kind_info->shared_ctl_off;
- else
+ /* initialize counter tracking */
+ if (kind_info->track_counts)
+ pg_atomic_init_u64(&ctl->entry_counts[kind - 1], 0);
+
+ /* initialize fixed-numbered stats */
+ if (kind_info->fixed_amount)
{
- int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
+ if (pgstat_is_kind_builtin(kind))
+ ptr = ((char *) ctl) + kind_info->shared_ctl_off;
+ else
+ {
+ int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
- Assert(kind_info->shared_size != 0);
- ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
- ptr = ctl->custom_data[idx];
+ Assert(kind_info->shared_size != 0);
+ ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
+ ptr = ctl->custom_data[idx];
+ }
+
+ kind_info->init_shmem_cb(ptr);
}
-
- kind_info->init_shmem_cb(ptr);
}
}
else
@@ -303,6 +311,7 @@ pgstat_init_entry(PgStat_Kind kind,
/* Create new stats entry. */
dsa_pointer chunk;
PgStatShared_Common *shheader;
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
/*
* Initialize refcount to 1, marking it as valid / not dropped. The entry
@@ -319,7 +328,7 @@ pgstat_init_entry(PgStat_Kind kind,
shhashent->dropped = false;
chunk = dsa_allocate_extended(pgStatLocal.dsa,
- pgstat_get_kind_info(kind)->shared_size,
+ kind_info->shared_size,
DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
if (chunk == InvalidDsaPointer)
return NULL;
@@ -330,6 +339,10 @@ pgstat_init_entry(PgStat_Kind kind,
/* Link the new entry from the hash entry. */
shhashent->body = chunk;
+ /* Increment entry count, if required. */
+ if (kind_info->track_counts)
+ pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
return shheader;
@@ -910,7 +923,17 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent,
/* release refcount marking entry as not dropped */
if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0)
{
+ PgStat_Kind kind = shent->key.kind;
+
pgstat_free_entry(shent, hstat);
+
+ /*
+ * Entry has been dropped with refcount at 0, hence decrement the
+ * entry counter.
+ */
+ if (pgstat_get_kind_info(kind)->track_counts)
+ pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
return true;
}
else
--
2.51.0
0002-injection_points-Add-entry-counting.patchtext/x-diff; charset=us-asciiDownload
From d92077bd939ac252e9dd245fec1e9470668411de Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 12 Sep 2025 16:12:11 +0900
Subject: [PATCH 2/2] injection_points: Add entry counting
This serves as coverage for the track_counts, as other in-core stats
kinds do not need to cap their maximum.
---
.../injection_points/injection_points--1.0.sql | 10 ++++++++++
src/test/modules/injection_points/injection_stats.c | 12 ++++++++++++
src/test/modules/injection_points/t/001_stats.pl | 12 ++++++++++++
3 files changed, 34 insertions(+)
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 5f5657b2043c..a7b61fbdfe6f 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -99,6 +99,16 @@ RETURNS bigint
AS 'MODULE_PATHNAME', 'injection_points_stats_numcalls'
LANGUAGE C STRICT;
+--
+-- injection_points_stats_count()
+--
+-- Return the number of entries stored in the pgstats hash table.
+--
+CREATE FUNCTION injection_points_stats_count()
+RETURNS bigint
+AS 'MODULE_PATHNAME', 'injection_points_stats_count'
+LANGUAGE C STRICT;
+
--
-- injection_points_stats_drop()
--
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index e3947b23ba57..15ad9883dedb 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -49,6 +49,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
.flush_pending_cb = injection_stats_flush_cb,
+ .track_counts = true,
};
/*
@@ -198,6 +199,17 @@ injection_points_stats_numcalls(PG_FUNCTION_ARGS)
PG_RETURN_INT64(entry->numcalls);
}
+/*
+ * SQL function returning the number of entries allocated for injection
+ * points in the shared hashtable of pgstats.
+ */
+PG_FUNCTION_INFO_V1(injection_points_stats_count);
+Datum
+injection_points_stats_count(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT64(pgstat_get_entry_count(PGSTAT_KIND_INJECTION));
+}
+
/* Only used by injection_points_stats_drop() */
static bool
match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data)
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 25de5fc46fe4..47ab58d0e9b8 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -36,6 +36,9 @@ $node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');");
my $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '2', 'number of stats calls');
+my $entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries');
my $fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls');
@@ -55,6 +58,9 @@ $node->restart;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '3', 'number of stats after clean restart');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries 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');
@@ -65,6 +71,9 @@ $node->start;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'number of stats after crash');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after crash');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
@@ -81,6 +90,9 @@ $node->safe_psql('postgres', "SELECT injection_points_stats_drop();");
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'no stats after drop via SQL function');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after drop via SQL function');
# Stop the server, disable the module, then restart. The server
# should be able to come up.
--
2.51.0
On Sep 12, 2025, at 15:23, Michael Paquier <michael@paquier.xyz> wrote:
--
Michael
<0001-Add-support-for-entry-counting-in-pgstats.patch><0002-injection_points-Add-entry-counting.patch>
The code overall looks good to me, very clear and neat. Just a few small comments:
1 - 0001
```
+ * set. This counter is incremented each time a new entry is created (not
+ * when reused), and each time an entry is dropped.
+ */
+ pg_atomic_uint64 entry_counts[PGSTAT_KIND_MAX];
```“and each time an entry is dropped” is a little misleading, it should be “and decremented when an entry is removed”.
2 - 0001
```
+ /* Increment entry count, if required. */
+ if (kind_info->track_counts)
+ pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
```The code is quite straightforward, so the comment seems unnecessary.
3 - 0001
```
+ if (kind_info->track_counts)
+ {
+ ereport(ERROR,
+ (errmsg("custom cumulative statistics property is invalid"),
+ errhint("Custom cumulative statistics cannot use counter tracking for fixed-numbered objects.")));
+ }
```{} are not needed. Looks like in the existing code, when “if” has a single statement, {} are not used. There is a similar “if” right above this change.
4 - 0004
```
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index e3947b23ba57..15ad9883dedb 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -49,6 +49,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
.flush_pending_cb = injection_stats_flush_cb,
+ .track_counts = true,
};
```Would it be better to move “.track_counts” up to be with the other boolean flags? Whey define together to share the same byte, feels better to initialize them together as well.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Thanks for this patch!
I have looked at what can be done, and finished with a rather simple
approach, as we have only one code path when a new entry is inserted,
and one code path when an entry is removed when the refcount of an
entry reaches 0 (includes resets, drops for kind matches, etc.). The
counters are stored in a uint64 atomic, one for each stats kind in
PgStat_ShmemControl, set only if the option is enabled.
The refcount reaches 0 when all backends release their references to the
stat, so if something like pg_stat_statements relies on a count for
deallocation purposes (to stay within the max), and that value only
decrements when all references are released, then it could end up
constantly triggering deallocation attempts. So, I am wondering if we
actually need another value that tracks "live" entries, or those that
have not yet been marked for drop. This means the live entries count
is decremented as soon as the entry is set to ->dropped.
Hence, a stats kind may want to protect
the entry deallocations with an extra lock, but that's not required
either depending on how aggressive removals should happen.
+1.
As a side note, maybe I am missing something here:
In [0]https://github.com/postgres/postgres/blob/f2bae51dfd5b2edc460c86071c577a45a1acbfd7/src/include/utils/pgstat_internal.h#L98-L99, should this not say ".... the entry should not be freed" instead of
".... the entry should not be dropped". The refcount deals with the freeing
of the entry after all refs are released.
--
Sami
On Tue, Sep 23, 2025 at 01:39:00PM -0500, Sami Imseih wrote:
The refcount reaches 0 when all backends release their references to the
stat, so if something like pg_stat_statements relies on a count for
deallocation purposes (to stay within the max), and that value only
decrements when all references are released, then it could end up
constantly triggering deallocation attempts. So, I am wondering if we
actually need another value that tracks "live" entries, or those that
have not yet been marked for drop. This means the live entries count
is decremented as soon as the entry is set to ->dropped.
Couldn't that mean a potential bloat in terms of memory in the dshash
if we have a large cycle of objects still held around but marked to be
gone? That sounds risky to me as it could go out of control. The
counter tracking exactly the number of dshash entries would provide a
tighter control, and perhaps this would be OK if the actual
deallocations are grouped. Your suggestion would not be difficult to
implement, as well, requiring an extra tweak when entries are reused,
and that's handled in a unique path.
As a side note, maybe I am missing something here:
In [0], should this not say ".... the entry should not be freed" instead of
".... the entry should not be dropped". The refcount deals with the freeing
of the entry after all refs are released.
The sentence looks correct to me? The refcount cannot be incremented
if an entry is marked for drop, as far as I recall.
--
Michael
Testing via the Injection Point has been successfully completed.
The option is named track_counts.
Regarding the option name track_counts in PgStat_KindInfo.
In my personal opinion, I was just wondering that it shares
the same name as the GUC track_counts(pgstat_track_counts in the source code).
If we want to make it clearer, renaming it to track_entry_counts
could be one option.
(That said, I feel the GUC track_counts option has a mismatch
between its role and its name...)
Best Regards,
--
Keisuke Kuroda
NTT DOCOMO SOLUTIONS, Inc.
On Wed, Sep 24, 2025 at 11:37:25AM +0900, Keisuke Kuroda wrote:
Regarding the option name track_counts in PgStat_KindInfo.
In my personal opinion, I was just wondering that it shares
the same name as the GUC track_counts(pgstat_track_counts in the source code).
If we want to make it clearer, renaming it to track_entry_counts
could be one option.
Yes, that's a good point and I have missed the GUC part. What you are
suggesting is cleaner overall with the flag added to the pgstats kind
info.
(That said, I feel the GUC track_counts option has a mismatch
between its role and its name...)
I doubt that we will rename this GUC at any point in the future.
Updated patch attached with the new name, as you are suggesting.
--
Michael
Attachments:
v2-0001-Add-support-for-entry-counting-in-pgstats.patchtext/x-diff; charset=us-asciiDownload
From 0b38ea6e3ef9cd30482983c0fda102472967b7dd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 25 Sep 2025 09:37:23 +0900
Subject: [PATCH v2 1/2] Add support for entry counting in pgstats
Stats kinds can set an option call track_counts, that will make pgstats
track the number of entries that exist in the shared hashtable.
As there is only one code path where a new entry is added, and one code
path where entries are removed, the count tracking is rather
straight-forward. Reads of these counters are optimistic, and may
change across two calls.
A use case of this facility is PGSS, where we need to be able to cap the
number of entries that would be stored in the shared hashtable, based on
its "max" GUC.
---
src/include/utils/pgstat_internal.h | 26 +++++++++++++
src/backend/utils/activity/pgstat.c | 6 +++
src/backend/utils/activity/pgstat_shmem.c | 47 +++++++++++++++++------
3 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index bf75ebcef31c..6daf20d0fefa 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -231,6 +231,12 @@ typedef struct PgStat_KindInfo
/* Should stats be written to the on-disk stats file? */
bool write_to_file:1;
+ /*
+ * Should the number of entries be tracked? For variable-numbered stats,
+ * to update its PgStat_ShmemControl.entry_counts.
+ */
+ bool track_entry_counts:1;
+
/*
* The size of an entry in the shared stats hash table (pointed to by
* PgStatShared_HashEntry->body). For fixed-numbered statistics, this is
@@ -500,6 +506,15 @@ typedef struct PgStat_ShmemControl
*/
pg_atomic_uint64 gc_request_count;
+ /*
+ * Counters for the number of entries associated to a single stats kind
+ * that uses variable-numbered objects stored in the shared hash table.
+ * These counters can be enabled on a per-kind basis, when track_counts is
+ * set. This counter is incremented each time a new entry is created (not
+ * when reused), and each time an entry is dropped.
+ */
+ pg_atomic_uint64 entry_counts[PGSTAT_KIND_MAX];
+
/*
* Stats data for fixed-numbered objects.
*/
@@ -925,6 +940,17 @@ pgstat_get_entry_data(PgStat_Kind kind, PgStatShared_Common *entry)
return ((char *) (entry)) + off;
}
+/*
+ * Returns the number of entries counted for a stats kind.
+ */
+static inline uint64
+pgstat_get_entry_count(PgStat_Kind kind)
+{
+ Assert(pgstat_get_kind_info(kind)->track_entry_counts);
+
+ return pg_atomic_read_u64(&pgStatLocal.shmem->entry_counts[kind - 1]);
+}
+
/*
* Returns a pointer to the shared memory area of custom stats for
* fixed-numbered statistics.
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 73c2ced3f4e2..6a53259b8d77 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1487,6 +1487,12 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info)
ereport(ERROR,
(errmsg("custom cumulative statistics property is invalid"),
errhint("Custom cumulative statistics require a shared memory size for fixed-numbered objects.")));
+ if (kind_info->track_entry_counts)
+ {
+ ereport(ERROR,
+ (errmsg("custom cumulative statistics property is invalid"),
+ errhint("Custom cumulative statistics cannot use counter tracking for fixed-numbered objects.")));
+ }
}
/*
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index ca36fd247f64..066027c387c4 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -210,27 +210,35 @@ StatsShmemInit(void)
pg_atomic_init_u64(&ctl->gc_request_count, 1);
- /* initialize fixed-numbered stats */
+ /* Do the per-kind initialization */
for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
char *ptr;
- if (!kind_info || !kind_info->fixed_amount)
+ if (!kind_info)
continue;
- if (pgstat_is_kind_builtin(kind))
- ptr = ((char *) ctl) + kind_info->shared_ctl_off;
- else
+ /* initialize counter tracking */
+ if (kind_info->track_entry_counts)
+ pg_atomic_init_u64(&ctl->entry_counts[kind - 1], 0);
+
+ /* initialize fixed-numbered stats */
+ if (kind_info->fixed_amount)
{
- int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
+ if (pgstat_is_kind_builtin(kind))
+ ptr = ((char *) ctl) + kind_info->shared_ctl_off;
+ else
+ {
+ int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
- Assert(kind_info->shared_size != 0);
- ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
- ptr = ctl->custom_data[idx];
+ Assert(kind_info->shared_size != 0);
+ ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
+ ptr = ctl->custom_data[idx];
+ }
+
+ kind_info->init_shmem_cb(ptr);
}
-
- kind_info->init_shmem_cb(ptr);
}
}
else
@@ -303,6 +311,7 @@ pgstat_init_entry(PgStat_Kind kind,
/* Create new stats entry. */
dsa_pointer chunk;
PgStatShared_Common *shheader;
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
/*
* Initialize refcount to 1, marking it as valid / not dropped. The entry
@@ -319,7 +328,7 @@ pgstat_init_entry(PgStat_Kind kind,
shhashent->dropped = false;
chunk = dsa_allocate_extended(pgStatLocal.dsa,
- pgstat_get_kind_info(kind)->shared_size,
+ kind_info->shared_size,
DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
if (chunk == InvalidDsaPointer)
return NULL;
@@ -330,6 +339,10 @@ pgstat_init_entry(PgStat_Kind kind,
/* Link the new entry from the hash entry. */
shhashent->body = chunk;
+ /* Increment entry count, if required. */
+ if (kind_info->track_entry_counts)
+ pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
return shheader;
@@ -907,7 +920,17 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent,
/* release refcount marking entry as not dropped */
if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0)
{
+ PgStat_Kind kind = shent->key.kind;
+
pgstat_free_entry(shent, hstat);
+
+ /*
+ * Entry has been dropped with refcount at 0, hence decrement the
+ * entry counter.
+ */
+ if (pgstat_get_kind_info(kind)->track_entry_counts)
+ pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
return true;
}
else
--
2.51.0
v2-0002-injection_points-Add-entry-counting.patchtext/x-diff; charset=us-asciiDownload
From 538e0bd0a896a914bfb157d8442b24c08b0345fc Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 25 Sep 2025 09:37:58 +0900
Subject: [PATCH v2 2/2] injection_points: Add entry counting
This serves as coverage for the track_counts, as other in-core stats
kinds do not need to cap their maximum.
---
.../injection_points/injection_points--1.0.sql | 10 ++++++++++
src/test/modules/injection_points/injection_stats.c | 12 ++++++++++++
src/test/modules/injection_points/t/001_stats.pl | 12 ++++++++++++
3 files changed, 34 insertions(+)
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 5f5657b2043c..a7b61fbdfe6f 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -99,6 +99,16 @@ RETURNS bigint
AS 'MODULE_PATHNAME', 'injection_points_stats_numcalls'
LANGUAGE C STRICT;
+--
+-- injection_points_stats_count()
+--
+-- Return the number of entries stored in the pgstats hash table.
+--
+CREATE FUNCTION injection_points_stats_count()
+RETURNS bigint
+AS 'MODULE_PATHNAME', 'injection_points_stats_count'
+LANGUAGE C STRICT;
+
--
-- injection_points_stats_drop()
--
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index ca8df4ad217a..ae6951372308 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -49,6 +49,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
.flush_pending_cb = injection_stats_flush_cb,
+ .track_entry_counts = true,
};
/*
@@ -196,6 +197,17 @@ injection_points_stats_numcalls(PG_FUNCTION_ARGS)
PG_RETURN_INT64(entry->numcalls);
}
+/*
+ * SQL function returning the number of entries allocated for injection
+ * points in the shared hashtable of pgstats.
+ */
+PG_FUNCTION_INFO_V1(injection_points_stats_count);
+Datum
+injection_points_stats_count(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT64(pgstat_get_entry_count(PGSTAT_KIND_INJECTION));
+}
+
/* Only used by injection_points_stats_drop() */
static bool
match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data)
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 25de5fc46fe4..47ab58d0e9b8 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -36,6 +36,9 @@ $node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');");
my $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '2', 'number of stats calls');
+my $entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries');
my $fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls');
@@ -55,6 +58,9 @@ $node->restart;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '3', 'number of stats after clean restart');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries 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');
@@ -65,6 +71,9 @@ $node->start;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'number of stats after crash');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after crash');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
@@ -81,6 +90,9 @@ $node->safe_psql('postgres', "SELECT injection_points_stats_drop();");
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'no stats after drop via SQL function');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after drop via SQL function');
# Stop the server, disable the module, then restart. The server
# should be able to come up.
--
2.51.0
On Tue, Sep 23, 2025 at 01:39:00PM -0500, Sami Imseih wrote:
The refcount reaches 0 when all backends release their references to the
stat, so if something like pg_stat_statements relies on a count for
deallocation purposes (to stay within the max), and that value only
decrements when all references are released, then it could end up
constantly triggering deallocation attempts. So, I am wondering if we
actually need another value that tracks "live" entries, or those that
have not yet been marked for drop. This means the live entries count
is decremented as soon as the entry is set to ->dropped.Couldn't that mean a potential bloat in terms of memory in the dshash
if we have a large cycle of objects still held around but marked to be
gone? That sounds risky to me as it could go out of control.
I spent a bit of time testing this with a pg_stat_statements like extension
using a custom stats kind, and while I think there is value for both "live"
( ! ->dropped) counter and an exact dshash counter ( current proposal),
I rather go with the latter at least initially, for the sake of not having 2
atomic counters. Both will allow an extension to trigger some type of a
cleanup strategy, and in general, both should be very close in value.
That's at least my observation.
I do think however that the placement of the decrement is wrong, and that
it should go inside pgstat_free_entry, since pgstat_free_entry can be called
in multiple code paths. In my high churn workload, v2 ended up increasing way
beyond the actual size of the dshash. See the attached for what
I did to fix.
Regarding the option name track_counts in PgStat_KindInfo.
In my personal opinion, I was just wondering that it shares
the same name as the GUC track_counts(pgstat_track_counts in the source code).
If we want to make it clearer, renaming it to track_entry_counts
could be one option.
Yes, that's a good point and I have missed the GUC part. What you are
suggesting is cleaner overall with the flag added to the pgstats kind
info.
IMO, "entry_counts" does not sound correct. We are not tracking more
than one count. What about track_num_entries ?
--
Sami
Attachments:
move_decrement.txttext/plain; charset=US-ASCII; name=move_decrement.txtDownload
index 066027c387c..2482e818e4c 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -872,6 +872,7 @@ static void
pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat)
{
dsa_pointer pdsa;
+ PgStat_Kind kind = shent->key.kind;
/*
* Fetch dsa pointer before deleting entry - that way we can free the
@@ -885,6 +886,11 @@ pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat)
dshash_delete_current(hstat);
dsa_free(pgStatLocal.dsa, pdsa);
+
+ /* Entry has been dropped, hence decrement the entry counter */
+ if (pgstat_get_kind_info(kind)->track_entry_counts)
+ pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
}
/*
@@ -920,17 +926,8 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent,
/* release refcount marking entry as not dropped */
if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0)
{
- PgStat_Kind kind = shent->key.kind;
-
pgstat_free_entry(shent, hstat);
- /*
- * Entry has been dropped with refcount at 0, hence decrement the
- * entry counter.
- */
- if (pgstat_get_kind_info(kind)->track_entry_counts)
- pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
-
return true;
}
elseOn Thu, Sep 25, 2025 at 07:47:48PM -0500, Sami Imseih wrote:
I spent a bit of time testing this with a pg_stat_statements like extension
using a custom stats kind, and while I think there is value for both "live"
( ! ->dropped) counter and an exact dshash counter ( current proposal),
I rather go with the latter at least initially, for the sake of not having 2
atomic counters. Both will allow an extension to trigger some type of a
cleanup strategy, and in general, both should be very close in value.
That's at least my observation.
Thanks for the feedback. I'm feeling encouraged by this opinion about
the "hard" counter, so I'd like to move on with it.
I do think however that the placement of the decrement is wrong, and that
it should go inside pgstat_free_entry, since pgstat_free_entry can be called
in multiple code paths. In my high churn workload, v2 ended up increasing way
beyond the actual size of the dshash. See the attached for what
I did to fix.
Yes, you are right and the patch is wrong. I've done the following
with my previous patch with injection_points loaded and its stats
activated, confirming your argument:
1) session with psql running in a tight loop and these queries:
select injection_points_attach('popo', 'notice');
select injection_points_run('popo');
select injection_points_detach('popo');
2) session that fetches the stats entry
select injection_points_stats_numcalls('popo');
\watch 0
3) session that checks the number of entries, should be 0 or 1:
select injection_points_stats_count();
\watch 1
And the numbers reported by the third session increase over time,
which is no good, once my patch is used. Once we move the
decrementation to pgstat_free_entry() as you propose, the counter is
under control and capped.
IMO, "entry_counts" does not sound correct. We are not tracking more
than one count. What about track_num_entries ?
Hmm. track_entry_counts resonates with the field name entry_counts,
and it's true that using the plural form is confusing. How about
track_entry_count in PgStat_KindInfo instead?
--
Michael
Attachments:
v3-0001-Add-support-for-entry-counting-in-pgstats.patchtext/x-diff; charset=us-asciiDownload
From 4761314ce27f8aaebaa531e8068513cf9924680b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 26 Sep 2025 12:53:27 +0900
Subject: [PATCH v3 1/2] Add support for entry counting in pgstats
Stats kinds can set an option call track_counts, that will make pgstats
track the number of entries that exist in the shared hashtable.
As there is only one code path where a new entry is added, and one code
path where entries are removed, the count tracking is rather
straight-forward. Reads of these counters are optimistic, and may
change across two calls.
A use case of this facility is PGSS, where we need to be able to cap the
number of entries that would be stored in the shared hashtable, based on
its "max" GUC.
---
src/include/utils/pgstat_internal.h | 27 +++++++++++++
src/backend/utils/activity/pgstat.c | 6 +++
src/backend/utils/activity/pgstat_shmem.c | 46 +++++++++++++++++------
3 files changed, 67 insertions(+), 12 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index bf75ebcef31c..c07efa1c8ebb 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -231,6 +231,12 @@ typedef struct PgStat_KindInfo
/* Should stats be written to the on-disk stats file? */
bool write_to_file:1;
+ /*
+ * Should the number of entries be tracked? For variable-numbered stats,
+ * to update its PgStat_ShmemControl.entry_counts.
+ */
+ bool track_entry_count:1;
+
/*
* The size of an entry in the shared stats hash table (pointed to by
* PgStatShared_HashEntry->body). For fixed-numbered statistics, this is
@@ -500,6 +506,16 @@ typedef struct PgStat_ShmemControl
*/
pg_atomic_uint64 gc_request_count;
+ /*
+ * Counters for the number of entries associated to a single stats kind
+ * that uses variable-numbered objects stored in the shared hash table.
+ * These counters can be enabled on a per-kind basis, when
+ * track_entry_count is set. This counter is incremented each time a
+ * new entry is created (not reused) in the shared hash table, and is
+ * decremented each time an entry is dropped from the shared hash table.
+ */
+ pg_atomic_uint64 entry_counts[PGSTAT_KIND_MAX];
+
/*
* Stats data for fixed-numbered objects.
*/
@@ -925,6 +941,17 @@ pgstat_get_entry_data(PgStat_Kind kind, PgStatShared_Common *entry)
return ((char *) (entry)) + off;
}
+/*
+ * Returns the number of entries counted for a stats kind.
+ */
+static inline uint64
+pgstat_get_entry_count(PgStat_Kind kind)
+{
+ Assert(pgstat_get_kind_info(kind)->track_entry_count);
+
+ return pg_atomic_read_u64(&pgStatLocal.shmem->entry_counts[kind - 1]);
+}
+
/*
* Returns a pointer to the shared memory area of custom stats for
* fixed-numbered statistics.
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 73c2ced3f4e2..5aee8a4ce1c8 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1487,6 +1487,12 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info)
ereport(ERROR,
(errmsg("custom cumulative statistics property is invalid"),
errhint("Custom cumulative statistics require a shared memory size for fixed-numbered objects.")));
+ if (kind_info->track_entry_count)
+ {
+ ereport(ERROR,
+ (errmsg("custom cumulative statistics property is invalid"),
+ errhint("Custom cumulative statistics cannot use counter tracking for fixed-numbered objects.")));
+ }
}
/*
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index ca36fd247f64..6c686d1615ba 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -210,27 +210,35 @@ StatsShmemInit(void)
pg_atomic_init_u64(&ctl->gc_request_count, 1);
- /* initialize fixed-numbered stats */
+ /* Do the per-kind initialization */
for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
char *ptr;
- if (!kind_info || !kind_info->fixed_amount)
+ if (!kind_info)
continue;
- if (pgstat_is_kind_builtin(kind))
- ptr = ((char *) ctl) + kind_info->shared_ctl_off;
- else
+ /* initialize counter tracking */
+ if (kind_info->track_entry_count)
+ pg_atomic_init_u64(&ctl->entry_counts[kind - 1], 0);
+
+ /* initialize fixed-numbered stats */
+ if (kind_info->fixed_amount)
{
- int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
+ if (pgstat_is_kind_builtin(kind))
+ ptr = ((char *) ctl) + kind_info->shared_ctl_off;
+ else
+ {
+ int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
- Assert(kind_info->shared_size != 0);
- ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
- ptr = ctl->custom_data[idx];
+ Assert(kind_info->shared_size != 0);
+ ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size);
+ ptr = ctl->custom_data[idx];
+ }
+
+ kind_info->init_shmem_cb(ptr);
}
-
- kind_info->init_shmem_cb(ptr);
}
}
else
@@ -303,6 +311,7 @@ pgstat_init_entry(PgStat_Kind kind,
/* Create new stats entry. */
dsa_pointer chunk;
PgStatShared_Common *shheader;
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
/*
* Initialize refcount to 1, marking it as valid / not dropped. The entry
@@ -319,7 +328,7 @@ pgstat_init_entry(PgStat_Kind kind,
shhashent->dropped = false;
chunk = dsa_allocate_extended(pgStatLocal.dsa,
- pgstat_get_kind_info(kind)->shared_size,
+ kind_info->shared_size,
DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
if (chunk == InvalidDsaPointer)
return NULL;
@@ -330,6 +339,10 @@ pgstat_init_entry(PgStat_Kind kind,
/* Link the new entry from the hash entry. */
shhashent->body = chunk;
+ /* Increment entry count, if required. */
+ if (kind_info->track_entry_count)
+ pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
+
LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
return shheader;
@@ -859,6 +872,7 @@ static void
pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat)
{
dsa_pointer pdsa;
+ PgStat_Kind kind = shent->key.kind;
/*
* Fetch dsa pointer before deleting entry - that way we can free the
@@ -872,6 +886,13 @@ pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat)
dshash_delete_current(hstat);
dsa_free(pgStatLocal.dsa, pdsa);
+
+ /*
+ * Entry has been dropped with refcount at 0, hence decrement the
+ * entry counter.
+ */
+ if (pgstat_get_kind_info(kind)->track_entry_count)
+ pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
}
/*
@@ -908,6 +929,7 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent,
if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0)
{
pgstat_free_entry(shent, hstat);
+
return true;
}
else
--
2.51.0
v3-0002-injection_points-Add-entry-counting.patchtext/x-diff; charset=us-asciiDownload
From e1f2bd22a0c80675179627ebcd3455e53129d9fd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Fri, 26 Sep 2025 12:54:15 +0900
Subject: [PATCH v3 2/2] injection_points: Add entry counting
This serves as coverage for the track_counts, as other in-core stats
kinds do not need to cap their maximum.
---
.../injection_points/injection_points--1.0.sql | 10 ++++++++++
src/test/modules/injection_points/injection_stats.c | 12 ++++++++++++
src/test/modules/injection_points/t/001_stats.pl | 12 ++++++++++++
3 files changed, 34 insertions(+)
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 5f5657b2043c..a7b61fbdfe6f 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -99,6 +99,16 @@ RETURNS bigint
AS 'MODULE_PATHNAME', 'injection_points_stats_numcalls'
LANGUAGE C STRICT;
+--
+-- injection_points_stats_count()
+--
+-- Return the number of entries stored in the pgstats hash table.
+--
+CREATE FUNCTION injection_points_stats_count()
+RETURNS bigint
+AS 'MODULE_PATHNAME', 'injection_points_stats_count'
+LANGUAGE C STRICT;
+
--
-- injection_points_stats_drop()
--
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index ca8df4ad217a..b47a3f367fff 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -49,6 +49,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
.flush_pending_cb = injection_stats_flush_cb,
+ .track_entry_count = true,
};
/*
@@ -196,6 +197,17 @@ injection_points_stats_numcalls(PG_FUNCTION_ARGS)
PG_RETURN_INT64(entry->numcalls);
}
+/*
+ * SQL function returning the number of entries allocated for injection
+ * points in the shared hashtable of pgstats.
+ */
+PG_FUNCTION_INFO_V1(injection_points_stats_count);
+Datum
+injection_points_stats_count(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_INT64(pgstat_get_entry_count(PGSTAT_KIND_INJECTION));
+}
+
/* Only used by injection_points_stats_drop() */
static bool
match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data)
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 25de5fc46fe4..47ab58d0e9b8 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -36,6 +36,9 @@ $node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');");
my $numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '2', 'number of stats calls');
+my $entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries');
my $fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls');
@@ -55,6 +58,9 @@ $node->restart;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '3', 'number of stats after clean restart');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '1', 'number of entries 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');
@@ -65,6 +71,9 @@ $node->start;
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'number of stats after crash');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after crash');
$fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
@@ -81,6 +90,9 @@ $node->safe_psql('postgres', "SELECT injection_points_stats_drop();");
$numcalls = $node->safe_psql('postgres',
"SELECT injection_points_stats_numcalls('stats-notice');");
is($numcalls, '', 'no stats after drop via SQL function');
+$entrycount =
+ $node->safe_psql('postgres', "SELECT injection_points_stats_count();");
+is($entrycount, '0', 'number of entries after drop via SQL function');
# Stop the server, disable the module, then restart. The server
# should be able to come up.
--
2.51.0
Thanks for v3. The only remaining comment I have is:
This comment seems unnecessary, since refcount is not checked inside
pgstat_free_entry, but earlier.
+ /*
+ * Entry has been dropped with refcount at 0, hence decrement the
+ * entry counter.
+ */I would just say this,
/* Decrement entry count, if required. */
--
Sami
On Fri, Sep 26, 2025 at 12:09:45PM -0500, Sami Imseih wrote:
Thanks for v3. The only remaining comment I have is:
Thanks for the extra lookup. I have fixed this one, incorporated the
feedback from Chao, and applied the result after more tests with
pgbench to check the state of the counter.
With more concurrency and an instance set up like that in
postgresql.conf:
shared_preload_libraries = 'injection_points'
injection_points.stats = on
And of course that:
psql -c 'create extension injection_points'
Then with the following flow, checking that the count never gets
higher than the number of clients:
1) pgbench -n -T 300 -f create_inj.sql -c 10
$ cat create_inj.sql
\set id random(1,100000)
select injection_points_attach('popo:id', 'notice');
select injection_points_run('popo:id');
select injection_points_detach('popo:id');
2) pgbench -n -T 300 -f select_inj.sql -c 10
$ cat select_inj.sql
\set id random(1,100000)
select injection_points_stats_numcalls('popo:id');
3) psql session:
select injection_points_stats_count();
\watch 1
And I have found, cough, a rather embarrassing bug on the way,
unrelated to the proposal of this thread. It's in the module
injection_points for its fixed stats, so nothing ground-breaking,
still embarrassing. Will post a fix on a new thread shortly..
--
Michael