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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Sami Imseih <samimseih(dot)pg(at)gmail(dot)com>
Cc: Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Lukas Fittl <lukas(at)fittl(dot)com>
Subject: Re: pgstat: Flush some statistics within running transactions, take 2
Date: 2026-09-09 03:50:55
Message-ID: aqDXnzW7R2Ky9zUC@paquier.xyz
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Moving truncdropped out is a net plus. That's a nice suggestion. And
that eases the two pg_memory_is_all_zeros() required to compare the
counter parts of the pending entries.

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

+ * 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.

Please note that I have done that while reading 0002, which should not
be impacted as all the non-transaction and transactional data still
have to go through PgStat_TableCounts.tab.

All that said, I finish with the attached for 0001 (+commit message
edits). How does this part look?
--
Michael

Attachment Content-Type Size
v14-0001-Split-transaction-data-for-pgstat-table-counters.patch text/plain 18.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2026-09-09 03:51:56 Re: Fix failing assert in deferred constraint trigger
Previous Message Jingtang Zhang 2026-09-09 03:41:55 Re: [PATCH] Use maintenance_io_concurrency for parallel index builds