Re: question about pending updates in pgstat_report_inj

Hi,

On 2025-09-15 18:21:22 -0400, Andres Freund wrote:
> On September 15, 2025 6:11:34 PM EDT, Sami Imseih <samimseih(at)gmail(dot)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