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

Hi,

On Thu, Jul 30, 2026 at 02:41:58PM -0500, Sami Imseih wrote:
> Hi,
>
> The discussion for mid-transaction flushes ended up taking place in
> the pg_stat_statements scalability thread [1], but I rather not have
> that thread become the place to review prerequisite patches. To keep
> things easier to follow, I am moving this patch back to its dedicated
> thread. The pg_stat_statements
> scalability work will follow as a patch set on top of this once this
> settles.
>
> Attaching v2 of this patch.

Thanks for the new version!

A few comments:

=== 1

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

That causes issues for function stats. pgstat_function_flush_cb() returns
PGSTAT_FLUSH_DONE, then pgstat_delete_pending_entry() is called, freeing storage
still referenced by PgStat_FunctionCallUsage.fs. When the function returns,
pgstat_end_function_usage() writes through that stale pointer.

Example:

postgres=# SET track_functions = 'all';

postgres=# CREATE FUNCTION force_flush_wrapper()
RETURNS void
LANGUAGE plpgsql
AS $$
BEGIN
PERFORM pg_stat_force_next_flush();
END
$$;

postgres=# SELECT force_flush_wrapper();
postgres=# SELECT force_flush_wrapper();

postgres=# SELECT funcname, calls
FROM pg_stat_user_functions
WHERE funcid = 'force_flush_wrapper()'::regprocedure;
funcname | calls
---------------------+-------
force_flush_wrapper | 0
(1 row)

We got 0 instead of 2 but that could be worst and corrupt memory if the freed
storage is reused.

Function is a special case, see:

"
typedef struct PgStat_FunctionCallUsage
{
/* Link to function's hashtable entry (must still be there at exit!) */
/* NULL means we are not tracking the current function call */
PgStat_FunctionCounts *fs;
"

but custom statistics can have the same problem if they keep a pointer to pending
data across pgstat_force_next_flush().

I think that entry_ref->pending should not be freed by an in-transaction flush.

=== 2

- bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
+ PgStat_FlushResult(*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait,
+ bool xact_boundary);

should we do the same for flush_static_cb() or exclude static callbacks from
mid-transaction flushes? (for the same reason that xact_boundary has been added
to flush_pending_cb())

=== 3

+typedef enum PgStat_FlushResult
+{
+ /* Fully flushed; the entry can be removed from the pending list. */
+ PGSTAT_FLUSH_DONE,
+
+ /*
+ * The lock could not be acquired without waiting (nowait was true). The
+ * entry must stay pending and be retried later.
+ */
+ PGSTAT_FLUSH_LOCK_CONFLICT,
+
+ /*
+ * Only part of the entry was flushed; some state was intentionally
+ * retained because it cannot be flushed in the current context (e.g.
+ * transactional counters flushed mid-transaction). The entry must stay
+ * pending and be flushed again at a suitable boundary.
+ */
+ PGSTAT_FLUSH_PARTIAL,
+} PgStat_FlushResult;

Previously, flush_pending_cb returned a bool with that meaning: false for lock
conflict and true for done.

Now 0 is PGSTAT_FLUSH_DONE and 1 is PGSTAT_FLUSH_LOCK_CONFLICT.

I think that would be better to change the enum ordering to preserve the previous
meaning. The concern is silent misbehavior in custom callbacks.

Regards,

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