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
