pgstat: Flush some statistics within running transactions, take 2

Hi,

The discussion for mid-transaction flushes ended up taking place in
the pg_stat_statements scalability thread [1]/messages/by-id/CAA5RZ0sQ+gDn-J85j1FzOdL1YjVYRegpmQpDiah1=REWZSZj+Q@mail.gmail.com, 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. It incorporates the feedback from the
discussion in the scalability thread [1]/messages/by-id/CAA5RZ0sQ+gDn-J85j1FzOdL1YjVYRegpmQpDiah1=REWZSZj+Q@mail.gmail.com.

Most of the structural changes here come from Kyotaro's review.
In v1 I pushed back on the context flag and the return value
semantics, but after sitting with it I think he was right on both
counts. The two points I adopted:

- The flush_pending_cb signature now takes an explicit xact_boundary
flag. As Bertrand argued, this makes the changed contract visible
to extensions implementing custom stats kinds, rather than relying
on the callback to infer it from internal state.

- The callback returns a PgStat_FlushResult enum (DONE,
LOCK_CONFLICT, PARTIAL) instead of giving the return bool multiple
meanings. The caller combines this with the flush context to decide
whether to remove the entry from the pending list. This separates
"intentionally partial flush due to mid-transaction flushes" from
"partially flushed due to lock conflict." Currently both cases will
require the same action to be taken, namely to retry the flush,
but keeping them separate makes it straightforward to handle them
differently in the future if needed.

Also changed from v1, the existing pg_stat_force_next_flush() is
extended instead of introducing pg_stat_report_anytime(). When called
in-transaction, it calls pgstat_report_stat(true) directly.

As before, only non-transactional counters are flushed
mid-transaction when a relation has active transaction state. In v2
however, the decision is based on both the xact_boundary flag and
lstats->trans != NULL.

lastscan uses GetCurrentStatementStartTimestamp() for in-transaction
flushes since GetCurrentTransactionStopTimestamp() is not available
in that context. v2 updates the docs for last_seq_scan/last_idx_scan
to reflect this.

Also v2 adds more comprehensive testing, including for ROLLBACK and
TRUNCATE.

As a follow-up item, I am also attaching an example that shows how
the flush API can be used to automatically flush non-transactional
stats at statement boundaries during long-running transactions,
throttled to once every 10 seconds. This is not part of the main
proposal, just a demonstration of what becomes possible on top of it.

[1]: /messages/by-id/CAA5RZ0sQ+gDn-J85j1FzOdL1YjVYRegpmQpDiah1=REWZSZj+Q@mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)

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

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 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;

Since the entry stays alive mid-transaction, callbacks that return
PGSTAT_FLUSH_DONE must zero their local counters after flushing to
avoid double-counting on the next flush. This was always the right
thing to do, but the immediate freeing of the entry did not make
this a strict requirement, and only pgstat_database_flush_cb already
did this. Now it's required for mid-transaction flushes, so I added memset()
after flush in pgstat_function_flush_cb, pgstat_relation_flush_cb,
pgstat_subscription_flush_cb, and the test_custom_stats example.

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())

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.

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

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

Agreed. In v3 I reordered the enum so PGSTAT_FLUSH_LOCK_CONFLICT is
first, with an explicit = 0 and a comment explaining why.

I also added a regression test that exercises the scenario you
described, a function calling pg_stat_force_next_flush() from within
itself.

One caveat is zeroing total_time can cause double-counting if a
recursive function calls pg_stat_force_next_flush(). This isn't
ideal, but I don't think we should handle more of these edge
cases for pg_stat_force_next_flush(). The only way I can think
of dealing with this is to flush total_time at xact_boundary, but
that will need to be done for all cases, which is not ideal, IMO.

v3 attached.

--
Sami Imseih
Amazon Web Services (AWS)

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

Thanks for the findings!

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.

I only looked at this finding so far. After spending some time
thinking about it, doing cross-kind accumulation inside
flush_pending_cb seems problematic by design.

```
bool
pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
...
....
/* The entry was successfully flushed, add the same to database stats */
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;
```

Right now with only flushing at the end of transaction, we can get
away with it because flushed entries are deleted, so dependent
entries get re-created at the tail and flushed in the same pass.
But the mid-transaction case exposes the ordering dependency, since
entries are not deleted and not re-visited in the same pass.
As you call out, custom stats could hit this too.

I think the fix is to separate cross-kind accumulation from the
flush itself. Rather than having flush_pending_cb call
pgstat_prep_database_pending() while we're iterating the pending
list, what do you think of adding a post_flush_pending_cb that
runs after a successful flush? At a transaction boundary this
works naturally. The flushed relation entry gets deleted, the
callback re-creates the database entry, and is guaranteed to
be visited.

For mid-transaction flushing where entries stay on the list, a
dependent entry that was already visited won't be reached again
in the first pass because it's not a new entry. So, if we are
mid-transaction and still have pending data, we can take a second
pass to handle the stats accumulated during the post flush
callback.

Adding post_flush_pending_cb can go in as a pre-requisite
commit. This is also better in terms of separation of
responsibilities between flushing the kinds stats and post
flush actions.

What do you think?

--
Sami

Hi,

On Tue, Aug 04, 2026 at 09:49:30PM -0500, Sami Imseih wrote:

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.

I think the fix is to separate cross-kind accumulation from the
flush itself. Rather than having flush_pending_cb call
pgstat_prep_database_pending() while we're iterating the pending
list, what do you think of adding a post_flush_pending_cb that
runs after a successful flush? At a transaction boundary this
works naturally. The flushed relation entry gets deleted, the
callback re-creates the database entry, and is guaranteed to
be visited.

One thing is that v3 flush_pending_cb clears the fields it publishes before
returning, so the post-flush callback would need the flushed delta to remain
available.

For mid-transaction flushing where entries stay on the list, a
dependent entry that was already visited won't be reached again
in the first pass because it's not a new entry. So, if we are
mid-transaction and still have pending data, we can take a second
pass to handle the stats accumulated during the post flush
callback.

I'm not sure one additional pass is enough for custom stats.
For example, say the list order is C, B, A, with A updating B and B updating C.
The first pass updates B, the second pass flushes B and updates the already-visited C,
a third pass is then needed.

Also, if "still have pending data" means "have_pending", that would not identify
newly generated work, as retained entries can keep it true. It seems that newly
generated work needs to be tracked and requeued separately.

Adding post_flush_pending_cb can go in as a pre-requisite
commit. This is also better in terms of separation of
responsibilities between flushing the kinds stats and post
flush actions.

I'm not sure a new callback is needed here. The relation callback already performs
the database accumulation only after a successful flush. If pending lifetime
and queue membership are separated, the existing pgstat_prep_database_pending()
call could requeue the database entry and preserve the current behavior.

I think that a post-flush callback might improve organization, but that seems
independent of this issue and so would need to be justified on its own.

Regards,

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

Adding post_flush_pending_cb can go in as a pre-requisite
commit. This is also better in terms of separation of
responsibilities between flushing the kinds stats and post
flush actions.

I'm not sure a new callback is needed here. The relation callback already performs
the database accumulation only after a successful flush. If pending lifetime
and queue membership are separated, the existing pgstat_prep_database_pending()
call could requeue the database entry and preserve the current behavior.

For now, I moved away from the extra callback idea, although I do think it
has benefits which could be hashed out in a different thread. The relation
callback still calls pgstat_prep_database_pending().

Rather than requeue individual entries, whenever a flush_pending_cb()
accumulates to another pending entry as is the case with relation -> database,
it sets a new global pgStatPendingFlushExtra = true, which then triggers
a secondary re-scan. We continue doing secondary re-scans until
pgStatPendingFlushExtra is back to false, so this can handle arbitrary numbers
of cascading pending entries, such as the case you raise earlier. This rescan
will only occur when we are mid-transaction, as it has no purpose on transaction
boundary.

I felt this is simpler than requeueing. Because each flush is a delta against
the flushed baseline, re-flushing an unchanged entry is a no-op, so the rescan
is safe without tracking per-entry queue membership; a callback only sets
pgStatPendingFlushExtra rather than managing the pending list. The rescan does
revisit entries with nothing new, but it only runs mid-transaction, so the
normal transaction boundary flush path is unaffected.

I also added an ample amount of test coverage in stats.sql and also testing
the cascading cases in test_custom_var_stats. I do create 2 new kinds to
test the cascade, which means we need to reserve 2 new Kind IDs.
If that is a problem, we can leave these tests out perhaps?

```
+/*
+ * Kind IDs for cascade flush test (A -> B -> C).
+ * Tests that pgStatPendingFlushExtra handles multi-level dependencies.
+ */
+#define PGSTAT_KIND_CASCADE_B 27
+#define PGSTAT_KIND_CASCADE_C 28
```

I think that a post-flush callback might improve organization, but that seems
independent of this issue and so would need to be justified on its own.

Yes, I still think there is value here, but I will table it for another idea.

With regards to the earlier comments.

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.

done.

=== 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.

done

=== 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?

You're correct.

Attached is v4.

--
Sami Imseih
Amazon Web Services (AWS)

Hi,

On Wed, Aug 05, 2026 at 08:22:55PM -0500, Sami Imseih wrote:

I also added an ample amount of test coverage in stats.sql and also testing
the cascading cases in test_custom_var_stats.

I'm not sure this test exercises the need for more than one extra pass.

It creates the cascade entries in the order B, C, A. So, during the first
extra pass, B is processed before C and C can be flushed during that same
pass.

Also, B and C are checked only after COMMIT, so the transaction end flush
could hide a failure of the in transaction rescan.

What about creating the entries in the order C, B, A and checking B and C
before COMMIT?

I do create 2 new kinds to
test the cascade, which means we need to reserve 2 new Kind IDs.
If that is a problem, we can leave these tests out perhaps?

```
+/*
+ * Kind IDs for cascade flush test (A -> B -> C).
+ * Tests that pgStatPendingFlushExtra handles multi-level dependencies.
+ */
+#define PGSTAT_KIND_CASCADE_B 27
+#define PGSTAT_KIND_CASCADE_C 28
```

Do we need separate kinds? Could B and C be represented as two objects belonging
to one custom kind?

A few more comments:

=== 1

pgstat_relation_flush_cb() now does:

+ tabentry->dead_tuples += lstats->counts.delta_dead_tuples - lstats->flushed.delta_dead_tuples;

and then records the cumulative value as flushed:

+	/*
+	 * Record what was flushed.  Transactional counters are retained until the
+	 * transaction boundary.
+	 */
+	if (flush_txn)
+	{
+		lstats->flushed = lstats->counts;

But pgstat_report_analyze() still subtracts the whole counts.delta_dead_tuples
value, so it can remove dead tuple statistics that were already published.

I think that pgstat_report_analyze() should subtract only the unflushed delta,
means:

deadtuples -= rel->pgstat_info->counts.delta_dead_tuples - rel->pgstat_info->flushed.delta_dead_tuples;

=== 2

The truncate handling resets the flushed.changed_tuples value:

+if (lstats->counts.truncdropped && !lstats->flushed.truncdropped)
+{
+	tabentry->live_tuples = 0;
+	tabentry->dead_tuples = 0;
+	tabentry->ins_since_vacuum = 0;
+	lstats->flushed.delta_live_tuples = 0;
+	lstats->flushed.delta_dead_tuples = 0;
+	lstats->flushed.changed_tuples = 0;
+}

However, changed_tuples is cumulative and truncate does not reset the shared
mod_since_analyze counter. Resetting flushed.changed_tuples to zero can
therefore publish changes that were already published.

I don't think flushed.changed_tuples should be reset here.

=== 3

The relation statistics reset is prevented by:

+if (lstats->counts.truncdropped && !lstats->flushed.truncdropped)

After the first full flush, this copies counts into flushed:

+	if (flush_txn)
+	{
+		lstats->flushed = lstats->counts;
+		return PGSTAT_FLUSH_DONE;
+	}

Both truncdropped values are then true. A subsequent truncate leaves
counts.truncdropped true, so the condition does not become true again and the
statistics reset is skipped.

What about doing this instead?

"
if (flush_txn)
{
lstats->flushed = lstats->counts;

lstats->counts.truncdropped = false;
lstats->flushed.truncdropped = false;

return PGSTAT_FLUSH_DONE;
}
"

=== 4

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

that means that now due to the extra loop:

+   /*
+    * Second scan (see above) for dependent entries populated after they were
+    * already visited.
+    */
+   while (pgStatPendingFlushExtra && IsTransactionOrTransactionBlock())
+   {

it invokes every retained callback again. Because pending->count is not cleared,
the same value is published twice. I think that can be an issue for custom
stats: the new test clears pending values:

+ memset(pending_entry, 0, sizeof(*pending_entry));

but that would be a new requirement for all custom callbacks.

At minimum that should be documented but I think a cleaner fix would be to
separate pending lifetime from queue membership, so that PGSTAT_FLUSH_DONE removes
an entry from the work queue while retaining its storage.

=== 5

In the extra scan, next is set before the callback:

+  next = dlist_has_next(&pgStatPending, cur) ?
+         dlist_next_node(&pgStatPending, cur) : NULL;
+
+  kind_info->flush_pending_cb(entry_ref, nowait, xact_boundary);

so, if the current tail callback creates a new dependent entry then it is processed
only if the callback also sets pgStatPendingFlushExtra, although the flag is
documented for updating an entry already visited. I think that next should be
set after the callback call (like the first scan).

=== 6

+   /*
+    * Second scan (see above) for dependent entries populated after they were
+    * already visited.
+    */
+   while (pgStatPendingFlushExtra && IsTransactionOrTransactionBlock())
+   {

What if:

A flushes into B and requests another pass
B flushes into A and requests another pass

Wouldn't that loop forever? That's not the case for core stats, but a custom
stats callback could create such a loop.

I'm not sure we can do much with the current global flag except document that
the dependencies must be acyclic.

Also, does that loop need CFI?

=== 7

+extern bool pgStatPendingFlushExtra;

missing PGDLLIMPORT?

=== 8

-  proparallel => 'r', prorettype => 'void', proargtypes => '',
+  proparallel => 'u', prorettype => 'void', proargtypes => '',

I think that would need a bump catalog version, add a XXX in the commit message
to not forget about it?

also "descr => 'statistics: force stats to be flushed after the next commit',"
should be updated?

=== 9

Should we also add test to verify that the function double counting bug is solved?

Regards,

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

Hi,

Thanks for the review!

At minimum that should be documented but I think a cleaner fix would be to
separate pending lifetime from queue membership, so that PGSTAT_FLUSH_DONE
removes an entry from the work queue while retaining its storage.

After discussing offline, I don't think I like the idea of an
extension having to set a flag to force a re-scan. It's not
a very clear API, so your idea of re-queueing and for this
to happen transparently is important. Initially, I was hesitant
to manipulate the pending list, but after playing around with this,
I think it's doable and the best way to proceed. We can
have a flag on entry_ref called flushed_this_pass which is
set to true whenever we flush it, even partially, but not on a
lock conflict, where nothing was flushed. Then, whenever
pgstat_prep_pending_from_entry_ref() is called again and the
entry is seen as having been flushed, we move it to the tail
so the ongoing scan can find it again.

```
@@ -1388,8 +1396,18 @@
pgstat_prep_pending_from_entry_ref(PgStat_EntryRef *entry_ref)
}

                entry_ref->pending =
MemoryContextAllocZero(pgStatPendingContext, entrysize);
+               entry_ref->flushed_this_pass = false;
                dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
        }
+       else if (entry_ref->flushed_this_pass)
+       {
+               /*
+                * The entry is already pending and was already
visited in the current
+                * flush pass.  Move it to the tail so the data just
accumulated into
+                * it is flushed again before the pass ends.
+                */
+               dlist_move_tail(&pgStatPending, &entry_ref->pending_node);
+       }
 }
```

That is the crux of the fix. The flush loop clears the flag on the
current entry before invoking its callback, so a callback that
accumulates into its own entry does not re-queue itself, and the next
pointer is determined after the callback returns, so a re-queued entry
is always picked up by the ongoing scan.

With regards to the still relevant points you raised:

==

I think that pgstat_report_analyze() should subtract only the unflushed
delta, means:

deadtuples -= rel->pgstat_info->counts.delta_dead_tuples - rel->pgstat_info->flushed.delta_dead_tuples;

done

==

However, changed_tuples is cumulative and truncate does not reset the shared
mod_since_analyze counter. Resetting flushed.changed_tuples to zero can
therefore publish changes that were already published.

I don't think flushed.changed_tuples should be reset here.

done.

===

Both truncdropped values are then true. A subsequent truncate leaves
counts.truncdropped true, so the condition does not become true again and
the statistics reset is skipped.

done.

===

I think that would need a bump catalog version, add a XXX in the commit
message to not forget about it?

also "descr => 'statistics: force stats to be flushed after the next
commit'," should be updated?

done. The catalog version is bumped and the descr now reads
"statistics: force stats to be flushed, immediately if within a
transaction".

===

Should we also add test to verify that the function double counting bug
is solved?

done. stats.sql now has a test where a plpgsql function calls
pg_stat_force_next_flush() from within itself, then verifies that
pg_stat_user_functions and pg_stat_get_xact_function_calls() both
report the correct call counts mid-transaction and after commit, with
no calls lost or double counted.

Also, I am keeping the test_custom_stats changes out of the main patch for
now, but have them attached as a nocfbot as they may help in the patch review.
The incorporate your ideas for the same kind with different objects, etc.

Lastly, Since the mid-transaction behavior is now user visible, v5 also brings
back the documentation updates for last_seq_scan/last_idx_scan from an
earlier version, and adds pg_stat_force_next_flush() to the statistics
functions table in monitoring.sgml. It was previously undocumented as a
test only helper, but that no longer seems appropriate given it now has
a public facing behavior worth describing.

Horighuchi-san raised a point here [1]/messages/by-id/20260601.135858.1116584574478485492.horikyota.ntt@gmail.com about throttling mid-transaction flushes,
but I am not sure if we should. These are manually executed, and I
think the caller
should be the one responsible for throttling, not the pgstat infrastructure.
WDYT?

Attached is v5.

[1]: /messages/by-id/20260601.135858.1116584574478485492.horikyota.ntt@gmail.com

--
Sami Imseih
Amazon Web Services (AWS)

Hi,

On Thu, Aug 06, 2026 at 06:23:53PM -0500, Sami Imseih wrote:

Hi,

That is the crux of the fix. The flush loop clears the flag on the
current entry before invoking its callback, so a callback that
accumulates into its own entry does not re-queue itself, and the next
pointer is determined after the callback returns, so a re-queued entry
is always picked up by the ongoing scan.

Thanks for the new version and explanation! Yeah, that makes sense to me for the
re queueing case.

A few comments:

=== 1

-       /* if successfully flushed, remove entry */
-       if (did_flush)
+       /*
+        * Never free pending entries mid-transaction; callers may hold
+        * pointers.
+        */
+       if (result == PGSTAT_FLUSH_DONE && xact_boundary)
            pgstat_delete_pending_entry(entry_ref);
        else
            have_pending = true;

This means that a callback returning PGSTAT_FLUSH_DONE keeps its pending entry
during a transaction.

The custom callback adds the complete pending value to shared stats but does not
clear or baseline it:

-   return true;
+   return PGSTAT_FLUSH_DONE;

So, the same value is published again by the next flush or at the transaction
boundary.

I think the callback should clear the published data, and that this new requirement
should be documented as a comment. Worth adding this case to 001_custom_stats.pl?

=== 2

 +       /*
 +        * Subtract the already-flushed instr_time baseline before converting to
 +        * microseconds, so that rounding does not drift across repeated flushes.
 +        */
 +       total_delta = localent->counts.total_time;
 +       INSTR_TIME_SUBTRACT(total_delta, localent->flushed.total_time);
...
 +       shfuncent->stats.total_time += INSTR_TIME_GET_MICROSEC(total_delta);
...
 +       localent->flushed = localent->counts;

I wonder if it wouldn't make more sense to subtract the converted cumulative
values instead, something like:

INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
INSTR_TIME_GET_MICROSEC(localent->flushed.total_time)

to avoid losing the fractional microseconds at each flush.

Horighuchi-san raised a point here [1] about throttling mid-transaction flushes,
but I am not sure if we should. These are manually executed, and I
think the caller
should be the one responsible for throttling, not the pgstat infrastructure.
WDYT?

=== 3

-       /* if successfully flushed, remove entry */
-       if (did_flush)
+       /*
+        * Never free pending entries mid-transaction; callers may hold
+        * pointers.
+        */
+       if (result == PGSTAT_FLUSH_DONE && xact_boundary)
            pgstat_delete_pending_entry(entry_ref);
        else
            have_pending = true;

so, fully flushed entries remain in pgStatPending until the transaction ends,
and each subsequent forced flush walks the list again from the head.

That means that a transaction touching one new entry and forcing a flush after
each step would execute 1 + 2 + ... + N callbacks. Function entries would also
acquire their shared entry lock again even when their delta is zero.

Do we expect the number of retained entries and forced flushes to remain small
enough for this not to matter? Given the intended pg_stat_statements use case,
maybe it would be worth benchmarking this before deciding that caller side
throttling is sufficient?

=== 4

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

IIUC, if a custom flush callback calls pgstat_force_next_flush(), this would
re enter pgstat_flush_pending_entries() and eventually invoke the same callback
again and again and again...

Should we add a "flush in progress" protection , or document and assert that
flush callbacks must not call pgstat_force_next_flush()?

=== 5

In the doc, the "Viewing Statistics" section still contains:

"
Each individual server process flushes out accumulated statistics to
shared memory just before going idle, but not more frequently than once
per PGSTAT_MIN_INTERVAL milliseconds ...
so a query or transaction still in progress does not affect the
displayed totals
"

Maybe this paragraph should be qualified as describing the normal automatic flushing
behavior and mention that an explicit forced flush is an exception?

Regards,

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

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.

```
+ struct
+ {
+ PgStat_IndexCounts counts;
+ PgStat_IndexCounts flushed;
+ } idx;
```

stats.sql now also tests index scans mid-transaction.

Responses inline.

=== 1
The custom callback adds the complete pending value to shared stats but
does not clear or baseline it:
...
I think the callback should clear the published data, and that this new
requirement should be documented as a comment. Worth adding this case to
001_custom_stats.pl?

Agreed. The custom callback now clears its pending counts right after adding
them to the shared entry.

```
/* Add pending counts to shared totals */
shared_entry->stats.numcalls += pending_entry->numcalls;

pgstat_unlock_entry(entry_ref);

+ /*
+ * The pending entry is retained across mid-transaction flushes (see
+ * pgstat_flush_pending_entries()), so clear what was just flushed to
+ * avoid counting it again on a later flush.  Nothing reads this stat
+ * within the current transaction, so zeroing is correct.
+ */
+ pending_entry->numcalls = 0;

return PGSTAT_FLUSH_DONE;
```

I also documented the requirement in the flush_pending_cb contract, since it
applies to any variable-numbered callback.

```
+ * When xact_boundary is false the pending entry is retained, so the
+ * callback may run again for it before the transaction ends and must not
+ * merge the same pending data twice.  Clear the flushed fields, or track
+ * a flushed baseline and merge only the delta.  A callback must not call
+ * pgstat_force_next_flush().
```

And 001_custom_stats.pl now forces a mid-transaction flush and checks the value
is counted once.

=== 2
I wonder if it wouldn't make more sense to subtract the converted
cumulative values instead, something like:
INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
INSTR_TIME_GET_MICROSEC(localent->flushed.total_time)
to avoid losing the fractional microseconds at each flush.

Right, converting the delta at each flush loses precision.
It now converts each cumulative value and subtracts in microseconds.

```
- total_delta = localent->counts.total_time;
- INSTR_TIME_SUBTRACT(total_delta, localent->flushed.total_time);
- shfuncent->stats.total_time += INSTR_TIME_GET_MICROSEC(total_delta);
+ shfuncent->stats.total_time +=
+ INSTR_TIME_GET_MICROSEC(localent->counts.total_time) -
+ INSTR_TIME_GET_MICROSEC(localent->flushed.total_time);
```

=== 3
That means that a transaction touching one new entry and forcing a flush after
each step would execute 1 + 2 + ... + N callbacks. Function entries would also
acquire their shared entry lock again even when their delta is zero.

Right, an unchanged entry should not take its lock. The function callback now
byte-compares its counts against the flushed baseline and returns
PGSTAT_FLUSH_DONE before taking the lock, matching the relation callback.

```
+ if (memcmp(&localent->counts, &localent->flushed,
+   sizeof(PgStat_FunctionCounts)) == 0)
+ return PGSTAT_FLUSH_DONE;
```

Do we expect the number of retained entries and forced flushes to remain small
enough for this not to matter? Given the intended pg_stat_statements use case,
maybe it would be worth benchmarking this before deciding that caller side
throttling is sufficient?

I would not say the number of retained entries or forced flushes stays small,
since both are up to the caller. The point is that this is opt-in. A
process that
flushes only at transaction boundary pays nothing, since entries are deleted at
the boundary as before with no re-walk. Only a caller that forces flushes
mid-transaction pays, and that cost is a memcmp per retained entry, not a lock.

On throttling, the caller controls that on their side by forcing fewer
flushes. To
make the re-walk cheaper we would keep a separate dirty list that the
flush walks
directly, but that means marking an entry dirty on the hot counting path, which
seems like a worse trade-off to me.

For the ongoing pg_stat_statements work, it flushes when
pg_stat_statements_internal() is called, and only in the backend
reading the view,
so that backend sees its own mid-transaction changes. For now this is
to make the
regression tests work. It is not a flush forced on every backend, and the common
case is still a flush at transaction boundary, so the re-walk does not become an
issue.

The larger goal is to let a transaction that runs many statements report its
non-transactional counters, whether from core or an extension, before the
transaction ends. That is the next step, and the patch in [1]/messages/by-id/CAA5RZ0u84eMFeFWMaEo0D84ed3jF_RY0=RWD8tDwQWsNsU1qvA@mail.gmail.com demonstrates it.

=== 4
IIUC, if a custom flush callback calls pgstat_force_next_flush(), this
would re enter pgstat_flush_pending_entries() ...
Should we add a "flush in progress" protection, or document and assert
that flush callbacks must not call pgstat_force_next_flush()?

Added the protection. pgstat_force_next_flush() now skips the immediate flush
while one is running and only sets the deferred flag.

```
+ /*
+ * When called inside a transaction, flush immediately.  Skip this if a
+ * flush is already running.
+ */
- if (IsTransactionOrTransactionBlock())
+ if (!pgStatFlushInProgress && IsTransactionOrTransactionBlock())
  pgstat_report_stat(true);

pgStatForceNextFlush = true;
```

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.

=== 5
Maybe this paragraph should be qualified as describing the normal
automatic flushing behavior and mention that an explicit forced flush is
an exception?

Done. Here is the new paragraph.

```
... so a query or transaction still in progress does not affect the
displayed totals and the displayed information lags behind actual
activity, unless the process is asked to flush its pending statistics
by calling pg_stat_force_next_flush().
```

What do you think?

[1]: /messages/by-id/CAA5RZ0u84eMFeFWMaEo0D84ed3jF_RY0=RWD8tDwQWsNsU1qvA@mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)

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

Thanks, Bertrand!

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

Agreed. My thinking was that a stale flag would self-heal, at the cost of
blocking only the next immediate in-transaction flush while the rest keeps
working. That is probably OK but not ideal, so I will add the
PG_TRY/PG_FINALLY and turn the reset at the start of pgstat_report_stat()
into 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;

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.

Good catch. Likely not a big deal in practice, but it points at something
more fundamental. PgStat_TableCounts mixes transactional and non-transactional
counters in no particular order, so a memcmp() of the whole struct cannot tell
which group changed.

I think we should keep a single struct but split it into two contiguous
regions, non-transactional first and transactional after, with comments
marking the boundary, and use an offset to compare each group on its own.
When flush_txn is false we then compare only the non-transactional group
before taking the lock, and return PGSTAT_FLUSH_PARTIAL when only the
deferred counters differ.

I would pull this out as a pre-req patch ahead of the main change. The
custom stats module could show the same pattern so extension developers are
mindful of this.

What do you think? I would also like to hear Michael's view on this before
I post the next revision.

--
Sami Imseih
Amazon Web Services (AWS)

Hi,

On Tue, Aug 18, 2026 at 01:01:25PM -0500, Sami Imseih wrote:

I think we should keep a single struct but split it into two contiguous
regions, non-transactional first and transactional after, with comments
marking the boundary, and use an offset to compare each group on its own.

FWIW, I'd vote for an helper comparing the five counters that can be flushed
immediately. There are only five, and adding another one would already require
updating the merge and baseline logic.

That would keep the policy local to pgstat_relation_flush_cb() and avoid making
PgStat_TableCounts field order part of the flush logic.

That said, let's see what Michael thinks.

Regards,

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

Thanks!

FWIW, I'd vote for an helper comparing the five counters that can be
flushed immediately. There are only five, and adding another one would
already require updating the merge and baseline logic.

That would keep the policy local to pgstat_relation_flush_cb() and avoid
making PgStat_TableCounts field order part of the flush logic.

Fair point.

I was looking at it less as a one-off fix here and more as a reusable approach
for kinds that mix the two. I'd rather not end up with a
field-by-field compare in
every flush callback, and just keep it a single memcmp(). I went with grouping
the counters and comparing by offset mainly because it was the least
friction to
get there, although not the cleanest.

What about splitting the transaction-safe and non-transaction-safe counters
into two nested structs inside PgStat_TableCounts? That puts the boundary in
the type instead of an offset, and each group is still one memcmp().

That said, let's see what Michael thinks.

+1

--
Sami Imseih
Amazon Web Services (AWS)

On Tue, Aug 18, 2026 at 10:50:48PM -0500, Sami Imseih wrote:

FWIW, I'd vote for an helper comparing the five counters that can be
flushed immediately. There are only five, and adding another one would
already require updating the merge and baseline logic.

That would keep the policy local to pgstat_relation_flush_cb() and avoid
making PgStat_TableCounts field order part of the flush logic.

Fair point.

I was looking at it less as a one-off fix here and more as a reusable approach
for kinds that mix the two. I'd rather not end up with a
field-by-field compare in
every flush callback, and just keep it a single memcmp(). I went with grouping
the counters and comparing by offset mainly because it was the least
friction to
get there, although not the cleanest.

What about splitting the transaction-safe and non-transaction-safe counters
into two nested structs inside PgStat_TableCounts? That puts the boundary in
the type instead of an offset, and each group is still one memcmp().

I agree that splitting the transaction and non-transactional parts of
PgStat_TableCounts, backend-level pending stats data for relations
would make sense. I was wondering about the interactions with
transactional flushes last week when splitting the relation and index
stats, with a single memcmp() not feeling like the best fit for the
job. Two memcmp() would feel better if we pass a transactional flag
to the flush callbacks.

It seems to me that you don't need two nested structures inside
PgStat_TableCounts (if that's what you mean?), but you could just have
a new piece for the transactional data in PgStat_RelationStatus when
dealing with a PGSTAT_KIND_RELATION? That feels simple enough as an
independent piece of refactoring, at quick glance.

That said, let's see what Michael thinks.

+1

Both of you are putting too much pressure on my shoulders. :)
--
Michael

Hi,

I posted v7, which addresses both Bertrand's earlier points and the
discussion about splitting the table stats counters into transactional
and non-transactional v7-0001 does the refactoring to split the table
stats and v7-0002 is now the in-transaction flush change.

I also expanded the custom stats example and TAP coverage in v7-0002
to show the same transactional and non-transactional split for
extension stats too.

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

Done in v7. pgstat_report_stat() now uses PG_TRY/PG_FINALLY to clear
pgStatFlushInProgress on all exit paths, and the reset at entry became
an assert. Initially, I thought that pgStatFlushInProgress would just
correct itself on the next flush, but this also means that
in-transaction, if there are consecutive flushes and the first one
errors out, the second one will not flush anything, but the third one
will, which is probably ok in practice, but not ideal.

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.

Done in v7. The relation flush path now compares only the
non-transactional group before taking the lock for an in-transaction
flush. If there are no changes in the non-transactional stats, it
returns PGSTAT_FLUSH_PARTIAL immediately.

I manually verified this fix because adding a test will require
an injection point, which I don't think is worthwhile.

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 */"

It can flush immediately.

Done in v7. I updated those comments to match the new behavior.

s/PgStat_TableStatus/PgStat_RelationStatus/?

Done in v7.

"pg_stat_force_next_flush() is not documented"

s/is not/is?

Done in v7.

--
Sami Imseih
Amazon Web Services (AWS)