Обсуждение: 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].
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_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
relationaccess. 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_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
relationaccess. 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 relationaccess. 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
Вложения
> 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