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
