Hi,
On Wed, Aug 12, 2026 at 01:13:14PM +0900, Michael Paquier wrote:
> On Mon, Aug 10, 2026 at 08:46:29AM +0000, Bertrand Drouvot wrote:
> > Thanks! I just had a look at the updated version and I can see that the
> > findings
> > reported above have been fixed.
>
> Okay, put my mind into this first part, and done.
Thanks!
> This puts in light a defect with the existing system views, at least
> it seems so to me. For example, pg_stat_xact_all_tables has a
> idx_tup_fetch, but no idx_tup_read equivalent, as we have in
> pg_stat_all_indexes. Perhaps this could be useful in terms of more
> xact-level metrics? Having all these functions is still required to
> me, of course. Or we could have a pg_stat_xact_all_indexes.
The additional functions make sense to me.
Yeah, I agree that there is a gap here. A pg_stat_xact_all_indexes view
looks like the natural way to expose these counters, and the new functions
provide the required data. I'd vote for handling that in a separate patch
though.
> A second thing is find_tabstat_entry_kind(), where I still have left
> the increments of tuples_inserted, tuples_updated and tuples_deleted
> for indexes on HEAD. That's a waste, but patch 2 takes care of that,
> so..
>
> Remaining patch attached. What do you think?
I've a few comments:
=== 1
.shared_size = sizeof(PgStatShared_Relation),
.shared_data_off = offsetof(PgStatShared_Relation, stats),
.shared_data_len = sizeof(((PgStatShared_Relation *) 0)->stats),
- .pending_size = sizeof(PgStat_TableStatus),
+ .pending_size = sizeof(PgStat_RelationStatus),
.flush_pending_cb = pgstat_relation_flush_cb,
.delete_pending_cb = pgstat_relation_delete_pending_cb,
@@ -326,7 +326,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_size = sizeof(PgStatShared_Index),
.shared_data_off = offsetof(PgStatShared_Index, stats),
.shared_data_len = sizeof(((PgStatShared_Index *) 0)->stats),
- .pending_size = sizeof(PgStat_TableStatus),
+ .pending_size = sizeof(PgStat_RelationStatus),
I agree that keeping a single Relation.pgstat_info pointer makes sense.
That said, IIUC, it does not require both kinds to use the same allocation
size.
I wonder if we could keep the single pointer design while using a small common
header in separate table and index pending structures? That would allow
Relation.pgstat_info to point to the common header while using a different
pending_size for each kind.
=== 2
+ union
+ {
+ /* table counters */
+ struct
+ {
+ Oid id; /* table's OID */
+ bool shared; /* is it a shared catalog? */
+ struct PgStat_RelXactStatus *trans; /* lowest subxact's counts */
+ PgStat_TableCounts counts; /* event counts to be sent */
+ } tab;
+
+ /* index counters */
+ PgStat_IndexCounts idx;
+ };
+} PgStat_RelationStatus;
The table and index specific functions and macros assume that the relation
has the expected kind. That assumption existed before this patch, but both
kinds previously used PgStat_TableStatus, so those accesses still referred to
fields that existed in the pending object.
With the new union, a wrong call can modify an unrelated member. For example,
pgstat_count_index_tuples() on a table updates idx.tuples_returned, which
overlaps the tab.trans pointer.
The same applies to:
pgstat_count_heap_scan()
pgstat_count_heap_getnext()
pgstat_count_index_scan()
pgstat_count_index_tuples()
pgstat_report_analyze()
pgstat_count_heap_insert()
pgstat_count_heap_update()
pgstat_count_heap_delete()
pgstat_count_truncate()
pgstat_update_heap_dead_tuples()
The generic pgstat_count_heap_fetch(), pgstat_count_buffer_read() and
pgstat_count_buffer_hit() already dispatch according to kind.
The current callers look correct, but worth adding assertions for the expected
kind in those functions and macros?
=== 3
-typedef struct PgStat_TableXactStatus
+typedef struct PgStat_RelXactStatus
{
PgStat_RelationStatus is common to tables and indexes, but PgStat_RelXactStatus
tracks transactional tuple changes for tables only.
Would keeping PgStat_TableXactStatus be less ambiguous here? It would also
match add_tabstat_xact_level() and ensure_tabstat_xact_level().
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
