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/
