> On Sat, Sep 05, 2026 at 05:09:57PM -0500, Sami Imseih wrote:
> > attached is the rebase.
>
> I have read through 0001, and put my hands on it.
>
> Based on what I have read, I am not much a fan of this way of
> splitting:
> +typedef struct PgStat_TableCounts
> +{
> + PgStat_TableCountsNonTxn nontxn;
> + PgStat_TableCountsTxn txn;
> } PgStat_TableCounts;
>
> This is adding one layer of structures but we do not actually need it,
> as what matters is if we are able to see changes in "tab" for a
> PgStat_RelationStatus, which is something that we have an access to in
> the flush callback.
>
> Instead, I think that we should extract the transactional counters out
> of PgStat_TableCounts into their own structure, and plug that directly
> into "tab". That's also less code churn as we need only to worry
> about the transaction part of the data moved around.
I feel it's better to keep them both under the "counts" since that
sounds like it encompasses both types of counters. I also get the
code churn argument and have no problem with proceeding this way.
> Naming the transactional data with "txn" was feeling a bit off, so I'd
> suggest a switch to "xact" to match with the SQL functions and the
> view definitions.
Works for me.
> There is little use for the two PGSTAT_ #defines
> outside the static assert, so just hardcoding the number is OK. No
> need to mention the StaticAssertDecl() in the comment: the no-padding
> rule is clear as water based on the static assert that documents the
> expectation.
I agree.
> + * numcalls is non-transactional and is flushed on any flush, including an
> + * in-transaction one. numcalls_txn is transactional and becomes visible in
> + * shared memory only at a transaction boundary; this demonstrates how a
> custom
> + * kind can defer transaction-dependent counters (see the flush callback).
>
> This comment in test_custom_var_stats.c is wrong. This whole diff
> should be in 0002.
I was attempting to explain the split of the counters here, but I see
your point. This can go in 0002.
overall v14-0001 LGTM.
--
Sami
