Hi, On September 15, 2025 6:11:34 PM EDT, Sami Imseih <[email protected]> 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.
