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

Hi,

On Fri, Jul 31, 2026 at 03:08:53PM -0500, Sami Imseih wrote:
> Hi,
>
> Thanks for the review!
>
> > 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.
>
> Good catch.
>
> > I think that entry_ref->pending should not be freed by an in-transaction flush.
>
> Agreed. In v3

Thanks for the new patch version!

Some comments:

=== 1

> I moved the protection into
> pgstat_flush_pending_entries() itself, so all callbacks are protected.

> if (result == PGSTAT_FLUSH_DONE && xact_boundary)
> pgstat_delete_pending_entry(entry_ref);
> else
> have_pending = true;

That produces a corner case for database and relation stats, for example:

"
CREATE TABLE flush_db_lag AS
SELECT i FROM generate_series(1, 100) AS g(i);

SELECT pg_stat_force_next_flush();

BEGIN;
SET LOCAL stats_fetch_consistency = none;
SELECT 1 FROM pg_class LIMIT 1;
SELECT pg_stat_force_next_flush();
SELECT pg_stat_reset();
SELECT pg_stat_get_db_tuples_returned(5) AS db_before,
pg_stat_get_tuples_returned(16384) AS rel_before;

SELECT count(*) FROM flush_db_lag;
SELECT pg_stat_force_next_flush();

SELECT pg_stat_get_db_tuples_returned(5) AS db_after_first,
pg_stat_get_tuples_returned(16384) AS rel_after_first;

SELECT pg_stat_force_next_flush();

SELECT pg_stat_get_db_tuples_returned(5) AS db_after_second,
pg_stat_get_tuples_returned(16384) AS rel_after_second;
"

Produces:

db_after_first | rel_after_first
----------------+-----------------
11 | 100

db_after_second | rel_after_second
-----------------+------------------
117 | 100

We can see that after the first force, relation statistics contains 100 tuples,
but the database aggregate does not (while the second force adds them).

This is because it keeps the flushed database entry in the pending list. When the
relation callback later adds counters to that already visited entry, it is not
requeued because entry_ref->pending is non-NULL. Then, those counters wait
until the next flush.

That could also happen for custom stats that updates an already visited retained
entry.

One option could be to keep pending entry memory allocated, but track queue
membership separately and requeue any entry that receives new counters after
being processed.

=== 2

> One caveat is zeroing total_time can cause double-counting if a
> recursive function calls pg_stat_force_next_flush().

That's also the case with consecutive nonrecursive calls, for example:

SET track_functions = 'all';

CREATE FUNCTION flush_timing_test(delay double precision)
RETURNS void LANGUAGE plpgsql AS $$
BEGIN
PERFORM pg_sleep(delay);
PERFORM pg_stat_force_next_flush();
END
$$;

BEGIN;
SELECT flush_timing_test(0.2);
SELECT flush_timing_test(0);
COMMIT;

SELECT pg_stat_force_next_flush();

SELECT calls, total_time, self_time
FROM pg_stat_user_functions
WHERE funcname = 'flush_timing_test';

produces:

calls | total_time | self_time
-------+------------+-----------
2 | 401.05 | 200.54

Maybe we could add a field that records how much of the cumulative value has
already been flushed.

=== 3

@@ -893,13 +929,29 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
dbentry = pgstat_prep_database_pending(dboid);
dbentry->tuples_returned += lstats->counts.tuples_returned;
dbentry->tuples_fetched += lstats->counts.tuples_fetched;
- dbentry->tuples_inserted += lstats->counts.tuples_inserted;
- dbentry->tuples_updated += lstats->counts.tuples_updated;
- dbentry->tuples_deleted += lstats->counts.tuples_deleted;
dbentry->blocks_fetched += lstats->counts.blocks_fetched;
dbentry->blocks_hit += lstats->counts.blocks_hit;

- return true;
+ if (flush_txn)
+ {
+ dbentry->tuples_inserted += lstats->counts.tuples_inserted;
+ dbentry->tuples_updated += lstats->counts.tuples_updated;
+ dbentry->tuples_deleted += lstats->counts.tuples_deleted;
+ memset(&lstats->counts, 0, sizeof(lstats->counts));

and

@@ -211,7 +217,14 @@ pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)

pgstat_unlock_entry(entry_ref);

- return true;
+ /*
+ * Zeroing total_time can cause double-counting if a recursive function
+ * calls pg_stat_force_next_flush(). This isn't ideal, but not worth
+ * adding complexity to handle that case.
+ */
+ memset(localent, 0, sizeof(*localent));

The two memset() means that we now clear the counters read by pg_stat_xact_*,
for example:

BEGIN;
SET LOCAL stats_fetch_consistency = none;
SELECT count(*) FROM xact_flush_rel;

SELECT seq_scan, seq_tup_read
FROM pg_stat_xact_user_tables
WHERE relname = 'xact_flush_rel';

seq_scan | seq_tup_read
----------+--------------
1 | 100

but:

SELECT pg_stat_force_next_flush();

Clears the counters:

SELECT seq_scan, seq_tup_read
FROM pg_stat_xact_user_tables
WHERE relname = 'xact_flush_rel';

seq_scan | seq_tup_read
----------+--------------
0 | 0

I think that the same solution for "=== 2" would work here: keep the counters
cumulative for the transaction, record how much has already been flushed, and
report only the difference.

=== 4

> Static callbacks (IO, WAL, SLRU, backend, lock) don't use
> entry_ref->pending, so there is no freeing risk.
> They also don't
> receive xact_boundary, so no changes were needed there.

The concern was not freeing: as flush_static_cb can now be called inside a
transaction, I think that it should receive xact_boundary or be skipped. That
would allow custom stats to also defer transaction dependent state.

=== 5

Should we mark pg_stat_force_next_flush() as PARALLEL UNSAFE, since the patch
can now invoke custom flush callbacks while a parallel query is active?

Regards,

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