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

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