Re: pgstat: Flush some statistics within running transactions, take 2

Hi,

On Thu, Aug 06, 2026 at 06:23:53PM -0500, Sami Imseih wrote:
> Hi,
>
> That is the crux of the fix. The flush loop clears the flag on the
> current entry before invoking its callback, so a callback that
> accumulates into its own entry does not re-queue itself, and the next
> pointer is determined after the callback returns, so a re-queued entry
> is always picked up by the ongoing scan.

Thanks for the new version and explanation! Yeah, that makes sense to me for the
re queueing case.

A few comments:

=== 1

- /* if successfully flushed, remove entry */
- if (did_flush)
+ /*
+ * Never free pending entries mid-transaction; callers may hold
+ * pointers.
+ */
+ if (result == PGSTAT_FLUSH_DONE && xact_boundary)
pgstat_delete_pending_entry(entry_ref);
else
have_pending = true;

This means that a callback returning PGSTAT_FLUSH_DONE keeps its pending entry
during a transaction.

The custom callback adds the complete pending value to shared stats but does not
clear or baseline it:

- return true;
+ return PGSTAT_FLUSH_DONE;

So, the same value is published again by the next flush or at the transaction
boundary.

I think the callback should clear the published data, and that this new requirement
should be documented as a comment. Worth adding this case to 001_custom_stats.pl?

=== 2

+ /*
+ * Subtract the already-flushed instr_time baseline before converting to
+ * microseconds, so that rounding does not drift across repeated flushes.
+ */
+ total_delta = localent->counts.total_time;
+ INSTR_TIME_SUBTRACT(total_delta, localent->flushed.total_time);
...
+ shfuncent->stats.total_time += INSTR_TIME_GET_MICROSEC(total_delta);
...
+ localent->flushed = localent->counts;

I wonder if it wouldn't make more sense to subtract the converted cumulative
values instead, something like:

INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
INSTR_TIME_GET_MICROSEC(localent->flushed.total_time)

to avoid losing the fractional microseconds at each flush.

> Horighuchi-san raised a point here [1] about throttling mid-transaction flushes,
> but I am not sure if we should. These are manually executed, and I
> think the caller
> should be the one responsible for throttling, not the pgstat infrastructure.
> WDYT?

=== 3

- /* if successfully flushed, remove entry */
- if (did_flush)
+ /*
+ * Never free pending entries mid-transaction; callers may hold
+ * pointers.
+ */
+ if (result == PGSTAT_FLUSH_DONE && xact_boundary)
pgstat_delete_pending_entry(entry_ref);
else
have_pending = true;

so, fully flushed entries remain in pgStatPending until the transaction ends,
and each subsequent forced flush walks the list again from the head.

That means that a transaction touching one new entry and forcing a flush after
each step would execute 1 + 2 + ... + N callbacks. Function entries would also
acquire their shared entry lock again even when their delta is zero.

Do we expect the number of retained entries and forced flushes to remain small
enough for this not to matter? Given the intended pg_stat_statements use case,
maybe it would be worth benchmarking this before deciding that caller side
throttling is sufficient?

=== 4

void
pgstat_force_next_flush(void)
{
+ if (IsTransactionOrTransactionBlock())
+ pgstat_report_stat(true);
+

IIUC, if a custom flush callback calls pgstat_force_next_flush(), this would
re enter pgstat_flush_pending_entries() and eventually invoke the same callback
again and again and again...

Should we add a "flush in progress" protection , or document and assert that
flush callbacks must not call pgstat_force_next_flush()?

=== 5

In the doc, the "Viewing Statistics" section still contains:

"
Each individual server process flushes out accumulated statistics to
shared memory just before going idle, but not more frequently than once
per PGSTAT_MIN_INTERVAL milliseconds ...
so a query or transaction still in progress does not affect the
displayed totals
"

Maybe this paragraph should be qualified as describing the normal automatic flushing
behavior and mention that an explicit forced flush is an exception?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com