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
