Thread
Commits
-
injection_points: Fix incrementation of variable-numbered stats
- 6e8286f9a153 18.0 landed
- 39f67d9b554f 19 (unreleased) landed
-
question about pending updates in pgstat_report_inj
Sami Imseih <samimseih@gmail.com> — 2025-09-15T22:11:34Z
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_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) -
Re: question about pending updates in pgstat_report_inj
Andres Freund <andres@anarazel.de> — 2025-09-15T22:21:22Z
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_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? 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. -
Re: question about pending updates in pgstat_report_inj
Andres Freund <andres@anarazel.de> — 2025-09-15T22:26:28Z
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_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? > > 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 -
Re: question about pending updates in pgstat_report_inj
Sami Imseih <samimseih@gmail.com> — 2025-09-15T22:33:45Z
> > >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
-
Re: question about pending updates in pgstat_report_inj
Michael Paquier <michael@paquier.xyz> — 2025-09-16T02:29:58Z
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
-
Re: question about pending updates in pgstat_report_inj
Sami Imseih <samimseih@gmail.com> — 2025-09-16T19:19:05Z
> 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
-
Re: question about pending updates in pgstat_report_inj
Michael Paquier <michael@paquier.xyz> — 2025-09-17T01:16:58Z
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