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

Hi,

On Fri, Aug 14, 2026 at 02:08:20PM -0500, Sami Imseih wrote:
> Hi,
>
> Thanks for the review.
>
> This version is rebased over 72a6dad1c9119c5. Index stats are now
> their own kind,
> so the patch also carries the counts/flushed delta calculations, as index stats
> can all be flushed mid-transaction.

Thanks!

> pgstat_report_stat() sets pgStatFlushInProgress while it flushes and resets it
> on entry, so a callback calling pgstat_force_next_flush() cannot re-enter. An
> error that leaves the flag set is harmless, since pgstat_report_stat() resets
> it on its next entry and a skipped force only defers the flush.

=== 1

+ pgStatFlushInProgress = true;
+
/* flush of variable-numbered stats tracked in pending entries list */
partial_flush |= pgstat_flush_pending_entries(nowait);

@@ -823,20 +836,24 @@ pgstat_report_stat(bool force)
if (!kind_info->flush_static_cb)
continue;

- partial_flush |= kind_info->flush_static_cb(nowait);
+ partial_flush |= kind_info->flush_static_cb(nowait,
+ !IsTransactionOrTransactionBlock());
}
}

+ pgStatFlushInProgress = false;

An ERROR raised by CHECK_FOR_INTERRUPTS() or by a flush callback would bypass
the final assignment. If it is caught in a subtransaction, the outer transaction
continues with pgStatFlushInProgress still true, so a later pg_stat_force_next_flush()
skips the immediate flush.

I wonder if pgStatFlushInProgress should be reset through PG_FINALLY? The reset
at the beginning of pgstat_report_stat() could then become an assert.

=== 2

+ if (memcmp(&lstats->tab.counts, &lstats->tab.flushed,
+ sizeof(struct PgStat_TableCounts)) == 0)
+ return flush_txn ? PGSTAT_FLUSH_DONE : PGSTAT_FLUSH_PARTIAL;
.
.
.
+ lstats->tab.flushed.numscans = lstats->tab.counts.numscans;
+ lstats->tab.flushed.tuples_returned = lstats->tab.counts.tuples_returned;
+ lstats->tab.flushed.tuples_fetched = lstats->tab.counts.tuples_fetched;
+ lstats->tab.flushed.blocks_fetched = lstats->tab.counts.blocks_fetched;
+ lstats->tab.flushed.blocks_hit = lstats->tab.counts.blocks_hit;

After a HOT or new-page update, every subsequent in transaction flush takes the
relation lock, even when there is nothing new to flush (because those deferred
counters keep the memcmp() unequal).

I wonder if, when flush_txn is false, we should check whether any of the non
transactional counters changed before taking the lock? If only deferred counters
differ, the callback could return PGSTAT_FLUSH_PARTIAL immediately.

=== 3

Some comments look stale:

"Once the stats are flushed, PgStat_EntryRef->pending is freed."

The pending entry can now be retained until the transaction boundary.

"
/* Force statistics to be reported at the next occasion */
Datum
pg_stat_force_next_flush(PG_FUNCTION_ARGS)
"

It can flush immediately.

+ * totals are never double-counted. The same counts/flushed scheme is used for
+ * relation stats; see PgStat_TableStatus.

and

+ * This struct should contain only actual event counters, because we byte
+ * compare it against the flushed baseline (see PgStat_TableStatus) to detect

s/PgStat_TableStatus/PgStat_RelationStatus/?

In the commit message:

"
pg_stat_force_next_flush() is not documented"

s/is not/is?

Regards,

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