question about pending updates in pgstat_report_inj
I have been reviewing how custom cumulative statistics should update their
counters, and noticed something unexpected in the injection_points example
[0]: https://www.postgresql.org/docs/18/xfunc-c.html
In pgstat_report_inj(), the code updates shared_stats directly:
```
shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
statent = &shstatent->stats;
/* Update the injection point statistics */
statent->numcalls++;
```
This works because it increments shared memory directly, but it bypasses the
usual pattern where updates go into ->pending and are later flushed into
shared memory by .flush_pending_cb
I think the more appropriate way to do this is:
```
pgstat_report_inj(const char *name)
{
PgStat_EntryRef *entry_ref;
- PgStatShared_InjectionPoint *shstatent;
PgStat_StatInjEntry *statent;
/* leave if disabled */
@@ -174,8 +173,7 @@ pgstat_report_inj(const char *name)
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,
PGSTAT_INJ_IDX(name), NULL);
- shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
- statent = &shstatent->stats;
+ statent = (PgStat_StatInjEntry *) entry_ref->pending; /* Update the injection point statistics */
statent->numcalls++;
```
which for example pgstat_report_subscription_error does something
similar.
Maybe I am missing something obvious for injection_points?
[0]: https://www.postgresql.org/docs/18/xfunc-c.html
--
Sami Imseih
Amazon Web Services (AWS)
Hi,
On September 15, 2025 6:11:34 PM EDT, Sami Imseih <samimseih@gmail.com> wrote:
I have been reviewing how custom cumulative statistics should update their
counters, and noticed something unexpected in the injection_points example
[0].In pgstat_report_inj(), the code updates shared_stats directly:
```
shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
statent = &shstatent->stats;/* Update the injection point statistics */
statent->numcalls++;
```This works because it increments shared memory directly, but it bypasses the
usual pattern where updates go into ->pending and are later flushed into
shared memory by .flush_pending_cbI think the more appropriate way to do this is:
```
pgstat_report_inj(const char *name)
{
PgStat_EntryRef *entry_ref;
- PgStatShared_InjectionPoint *shstatent;
PgStat_StatInjEntry *statent;/* leave if disabled */
@@ -174,8 +173,7 @@ pgstat_report_inj(const char *name)
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,PGSTAT_INJ_IDX(name), NULL);
- shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats; - statent = &shstatent->stats; + statent = (PgStat_StatInjEntry *) entry_ref->pending;/* Update the injection point statistics */
statent->numcalls++;
```which for example pgstat_report_subscription_error does something
similar.Maybe I am missing something obvious for injection_points?
The point of deferring updating shared stats is to avoid contention. That's certainly crucial for something like relation access. But it seems extremely unlikely that there would be contention due to injection point stat updates.
Greetings,
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi,
On 2025-09-15 18:21:22 -0400, Andres Freund wrote:
On September 15, 2025 6:11:34 PM EDT, Sami Imseih <samimseih@gmail.com> wrote:
I have been reviewing how custom cumulative statistics should update their
counters, and noticed something unexpected in the injection_points example
[0].In pgstat_report_inj(), the code updates shared_stats directly:
```
shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
statent = &shstatent->stats;/* Update the injection point statistics */
statent->numcalls++;
```This works because it increments shared memory directly, but it bypasses the
usual pattern where updates go into ->pending and are later flushed into
shared memory by .flush_pending_cbI think the more appropriate way to do this is:
```
pgstat_report_inj(const char *name)
{
PgStat_EntryRef *entry_ref;
- PgStatShared_InjectionPoint *shstatent;
PgStat_StatInjEntry *statent;/* leave if disabled */
@@ -174,8 +173,7 @@ pgstat_report_inj(const char *name)
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,PGSTAT_INJ_IDX(name), NULL);
- shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats; - statent = &shstatent->stats; + statent = (PgStat_StatInjEntry *) entry_ref->pending;/* Update the injection point statistics */
statent->numcalls++;
```which for example pgstat_report_subscription_error does something
similar.Maybe I am missing something obvious for injection_points?
The point of deferring updating shared stats is to avoid contention. That's certainly crucial for something like relation access. But it seems extremely unlikely that there would be contention due to injection point stat updates.
But, uh, the code is incorrect as-is, due to not locking the shared stats
while updating them. Which means it's entirely possible to loose stats updates
if updated by multipole backends. Am I missing something?
Greetings,
Andres Freund
Maybe I am missing something obvious for injection_points?
The point of deferring updating shared stats is to avoid contention. That's certainly crucial for something like relation access. But it seems extremely unlikely that there would be contention due to injection point stat updates.
But, uh, the code is incorrect as-is, due to not locking the shared stats
while updating them. Which means it's entirely possible to loose stats updates
if updated by multipole backends. Am I missing something?
yes, and that was going to be my next point. There is no locking while updating,
which is wrong.
Furthermore that flush pending callback,
injection_stats_flush_cb, is not required as we have it
defined. Well, it's required that we define it, but in this case it could
just return true, rather than trying to flush pending data that was
never accumulated.
I think it's better to use ->pending here, since this is referenced
as an example and most real-world cases will likely want to use
->pending for performance reasons.
--
Sami
On Mon, Sep 15, 2025 at 05:33:45PM -0500, Sami Imseih wrote:
I think it's better to use ->pending here, since this is referenced
as an example and most real-world cases will likely want to use
->pending for performance reasons.
Yes, it should use the pending entry. b757abefc041 did not get that
completely right.
The purpose of this code is also to serve as a template, so better
that it does the correct thing.
How about renaming "statent" to "pending" in pgstat_report_inj(), as
well? That would be a bit more consistent with the subscription stat
case, at least.
--
Michael
Attachments:
0001-injection_points-Fix-incrementation-of-variable-numb.patchtext/x-diff; charset=us-asciiDownload
From 3c0740039c07d2e7bd4ad101ea15f4630a0c4efa Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 16 Sep 2025 11:28:18 +0900
Subject: [PATCH] injection_points: Fix incrementation of variable-numbered
stats
The pending entry was not used when incrementing its data, the code was
directly manipulating the shared memory pointer, without even locking
it.
---
src/test/modules/injection_points/injection_stats.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index e3947b23ba57..ca8df4ad217a 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -164,8 +164,7 @@ void
pgstat_report_inj(const char *name)
{
PgStat_EntryRef *entry_ref;
- PgStatShared_InjectionPoint *shstatent;
- PgStat_StatInjEntry *statent;
+ PgStat_StatInjEntry *pending;
/* leave if disabled */
if (!inj_stats_loaded || !inj_stats_enabled)
@@ -174,11 +173,10 @@ pgstat_report_inj(const char *name)
entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,
PGSTAT_INJ_IDX(name), NULL);
- shstatent = (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
- statent = &shstatent->stats;
+ pending = (PgStat_StatInjEntry *) entry_ref->pending;
- /* Update the injection point statistics */
- statent->numcalls++;
+ /* Update the injection point pending statistics */
+ pending->numcalls++;
}
/*
--
2.51.0
On Mon, Sep 15, 2025 at 05:33:45PM -0500, Sami Imseih wrote:
I think it's better to use ->pending here, since this is referenced
as an example and most real-world cases will likely want to use
->pending for performance reasons.Yes, it should use the pending entry. b757abefc041 did not get that
completely right.The purpose of this code is also to serve as a template, so better
that it does the correct thing.How about renaming "statent" to "pending" in pgstat_report_inj(), as
well? That would be a bit more consistent with the subscription stat
case, at least.
0001 LGTM.
--
Sami
On Tue, Sep 16, 2025 at 02:19:05PM -0500, Sami Imseih wrote:
On Mon, Sep 15, 2025 at 05:33:45PM -0500, Sami Imseih wrote:
How about renaming "statent" to "pending" in pgstat_report_inj(), as
well? That would be a bit more consistent with the subscription stat
case, at least.0001 LGTM.
Okay. Applied this way, then.
--
Michael