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
From 3960e360b55c6f4ef1c8bb565a45b6d9f4057dfb Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Wed, 9 Sep 2026 12:33:33 +0900 Subject: [PATCH v14] Split transaction data for pgstat table counters PgStat_TableCounts currently stores all per table pending counters in one structure. This is now split into two parts, with the transactional part of the data moved out of PgStat_TableCounts into its own structure labelled with an "xact" in PgStat_RelationStatus. While on it, move truncdropped out of the counters and into PgStat_RelationStatus so as both structs contain only counters. This was slightly confusing, this flag tracks if a relation has been truncated at backend-level, which is no data. Because truncdropped no longer lives inside PgStat_TableCounts, this commit teaches the flush callback to treat a pending truncate as sufficient reason to flush, even when both counter structs are otherwise all zero. This refactoring work is in preparation of a different patch that wants to introduce the concept of transactional stats data flush. Compared to all the other stats kinds, relations are an exception as they mix transactional and non-transactional data. Author: Sami Imseih <[email protected]> Reviewed-by: Bertrand Drouvot <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/include/pgstat.h | 42 ++++++-- src/backend/utils/activity/pgstat_relation.c | 108 ++++++++++--------- src/backend/utils/adt/pgstatfuncs.c | 24 ++--- src/tools/pgindent/typedefs.list | 1 + 4 files changed, 104 insertions(+), 71 deletions(-) diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 204782fd630c..187d82c96fef 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -121,6 +121,10 @@ typedef struct PgStat_BackendSubEntry /* ---------- * PgStat_TableCounts The actual per-table counts kept by a backend * + * These counters are nontransactional: they are recorded whether the + * transaction commits or aborts. Counters whose effect depends on the + * transaction outcome are kept separately, in PgStat_TableCountsXact. + * * This struct should contain only actual event counters, because we make use * of pg_memory_is_all_zeros() to detect whether there are any stats updates * to apply. @@ -131,11 +135,6 @@ typedef struct PgStat_BackendSubEntry * Note: tuples_returned is the number of tuples successfully fetched by * heap_getnext, while tuples_fetched is the number of tuples successfully * fetched by heap_fetch under the control of bitmap indexscans. - * - * tuples_inserted/updated/deleted/hot_updated/newpage_updated count attempted - * actions, regardless of whether the transaction committed. delta_live_tuples, - * delta_dead_tuples, and changed_tuples are set depending on commit or abort. - * Note that delta_live_tuples and delta_dead_tuples can be negative! * ---------- */ typedef struct PgStat_TableCounts @@ -145,20 +144,43 @@ typedef struct PgStat_TableCounts PgStat_Counter tuples_returned; PgStat_Counter tuples_fetched; + PgStat_Counter blocks_fetched; + PgStat_Counter blocks_hit; +} PgStat_TableCounts; + +StaticAssertDecl(sizeof(PgStat_TableCounts) == 5 * sizeof(PgStat_Counter), + "PgStat_TableCounts has no padding"); + +/* ---------- + * PgStat_TableCountsXact Transactional per-table counts kept by a backend + * + * The same rules as for PgStat_TableCounts apply: this struct should contain + * only actual event counters + * + * It is a component of PgStat_RelationStatus (within-backend state, for + * table data). + * + * tuples_inserted/updated/deleted/hot_updated/newpage_updated count attempted + * actions, regardless of whether the transaction committed. delta_live_tuples, + * delta_dead_tuples, and changed_tuples are set depending on commit or abort. + * Note that delta_live_tuples and delta_dead_tuples can be negative! + * ---------- + */ +typedef struct PgStat_TableCountsXact +{ PgStat_Counter tuples_inserted; PgStat_Counter tuples_updated; PgStat_Counter tuples_deleted; PgStat_Counter tuples_hot_updated; PgStat_Counter tuples_newpage_updated; - bool truncdropped; PgStat_Counter delta_live_tuples; PgStat_Counter delta_dead_tuples; PgStat_Counter changed_tuples; +} PgStat_TableCountsXact; - PgStat_Counter blocks_fetched; - PgStat_Counter blocks_hit; -} PgStat_TableCounts; +StaticAssertDecl(sizeof(PgStat_TableCountsXact) == 8 * sizeof(PgStat_Counter), + "PgStat_TableCountsXact has no padding"); /* ---------- * PgStat_IndexCounts Per-index pending event counters @@ -210,8 +232,10 @@ typedef struct PgStat_RelationStatus { Oid id; /* table's OID */ bool shared; /* is it a shared catalog? */ + bool truncdropped; /* pending truncate/drop reset */ struct PgStat_TableXactStatus *trans; /* lowest subxact's counts */ PgStat_TableCounts counts; /* event counts to be sent */ + PgStat_TableCountsXact counts_xact; /* transactional counts */ } tab; /* index counters */ diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index 17746bf5c54f..5c70543ba88a 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -369,7 +369,7 @@ pgstat_report_analyze(Relation rel, deadtuples -= trans->tuples_updated + trans->tuples_deleted; } /* count stuff inserted by already-aborted subxacts, too */ - deadtuples -= rel->pgstat_info->tab.counts.delta_dead_tuples; + deadtuples -= rel->pgstat_info->tab.counts_xact.delta_dead_tuples; /* Since ANALYZE's counts are estimates, we could have underflowed */ livetuples = Max(livetuples, 0); deadtuples = Max(deadtuples, 0); @@ -459,9 +459,9 @@ pgstat_count_heap_update(Relation rel, bool hot, bool newpage) * nontransactional, so just advance them */ if (hot) - pgstat_info->tab.counts.tuples_hot_updated++; + pgstat_info->tab.counts_xact.tuples_hot_updated++; else if (newpage) - pgstat_info->tab.counts.tuples_newpage_updated++; + pgstat_info->tab.counts_xact.tuples_newpage_updated++; } } @@ -519,7 +519,7 @@ pgstat_update_heap_dead_tuples(Relation rel, int delta) Assert(pgstat_info->kind == PGSTAT_KIND_RELATION); - pgstat_info->tab.counts.delta_dead_tuples -= delta; + pgstat_info->tab.counts_xact.delta_dead_tuples -= delta; } } @@ -603,9 +603,9 @@ find_relstat_entry_kind(PgStat_Kind kind, Oid rel_id) */ for (trans = relentry->tab.trans; trans != NULL; trans = trans->upper) { - relstatus->tab.counts.tuples_inserted += trans->tuples_inserted; - relstatus->tab.counts.tuples_updated += trans->tuples_updated; - relstatus->tab.counts.tuples_deleted += trans->tuples_deleted; + relstatus->tab.counts_xact.tuples_inserted += trans->tuples_inserted; + relstatus->tab.counts_xact.tuples_updated += trans->tuples_updated; + relstatus->tab.counts_xact.tuples_deleted += trans->tuples_deleted; } return relstatus; @@ -636,33 +636,33 @@ AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit) if (!isCommit) restore_truncdrop_counters(trans); /* count attempted actions regardless of commit/abort */ - relstat->tab.counts.tuples_inserted += trans->tuples_inserted; - relstat->tab.counts.tuples_updated += trans->tuples_updated; - relstat->tab.counts.tuples_deleted += trans->tuples_deleted; + relstat->tab.counts_xact.tuples_inserted += trans->tuples_inserted; + relstat->tab.counts_xact.tuples_updated += trans->tuples_updated; + relstat->tab.counts_xact.tuples_deleted += trans->tuples_deleted; if (isCommit) { - relstat->tab.counts.truncdropped = trans->truncdropped; + relstat->tab.truncdropped = trans->truncdropped; if (trans->truncdropped) { /* forget live/dead stats seen by backend thus far */ - relstat->tab.counts.delta_live_tuples = 0; - relstat->tab.counts.delta_dead_tuples = 0; + relstat->tab.counts_xact.delta_live_tuples = 0; + relstat->tab.counts_xact.delta_dead_tuples = 0; } /* insert adds a live tuple, delete removes one */ - relstat->tab.counts.delta_live_tuples += + relstat->tab.counts_xact.delta_live_tuples += trans->tuples_inserted - trans->tuples_deleted; /* update and delete each create a dead tuple */ - relstat->tab.counts.delta_dead_tuples += + relstat->tab.counts_xact.delta_dead_tuples += trans->tuples_updated + trans->tuples_deleted; /* insert, update, delete each count as one change event */ - relstat->tab.counts.changed_tuples += + relstat->tab.counts_xact.changed_tuples += trans->tuples_inserted + trans->tuples_updated + trans->tuples_deleted; } else { /* inserted tuples are dead, deleted tuples are unaffected */ - relstat->tab.counts.delta_dead_tuples += + relstat->tab.counts_xact.delta_dead_tuples += trans->tuples_inserted + trans->tuples_updated; /* an aborted xact generates no changed_tuple events */ } @@ -742,11 +742,11 @@ AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, in /* first restore values obliterated by truncate/drop */ restore_truncdrop_counters(trans); /* count attempted actions regardless of commit/abort */ - relstat->tab.counts.tuples_inserted += trans->tuples_inserted; - relstat->tab.counts.tuples_updated += trans->tuples_updated; - relstat->tab.counts.tuples_deleted += trans->tuples_deleted; + relstat->tab.counts_xact.tuples_inserted += trans->tuples_inserted; + relstat->tab.counts_xact.tuples_updated += trans->tuples_updated; + relstat->tab.counts_xact.tuples_deleted += trans->tuples_deleted; /* inserted tuples are dead, deleted tuples are unaffected */ - relstat->tab.counts.delta_dead_tuples += + relstat->tab.counts_xact.delta_dead_tuples += trans->tuples_inserted + trans->tuples_updated; relstat->tab.trans = trans->upper; pfree(trans); @@ -826,21 +826,21 @@ pgstat_twophase_postcommit(FullTransactionId fxid, uint16 info, pgstat_info = pgstat_prep_relation_pending(PGSTAT_KIND_RELATION, rec->id, rec->shared); /* Same math as in AtEOXact_PgStat, commit case */ - pgstat_info->tab.counts.tuples_inserted += rec->tuples_inserted; - pgstat_info->tab.counts.tuples_updated += rec->tuples_updated; - pgstat_info->tab.counts.tuples_deleted += rec->tuples_deleted; - pgstat_info->tab.counts.truncdropped = rec->truncdropped; + pgstat_info->tab.counts_xact.tuples_inserted += rec->tuples_inserted; + pgstat_info->tab.counts_xact.tuples_updated += rec->tuples_updated; + pgstat_info->tab.counts_xact.tuples_deleted += rec->tuples_deleted; + pgstat_info->tab.truncdropped = rec->truncdropped; if (rec->truncdropped) { /* forget live/dead stats seen by backend thus far */ - pgstat_info->tab.counts.delta_live_tuples = 0; - pgstat_info->tab.counts.delta_dead_tuples = 0; + pgstat_info->tab.counts_xact.delta_live_tuples = 0; + pgstat_info->tab.counts_xact.delta_dead_tuples = 0; } - pgstat_info->tab.counts.delta_live_tuples += + pgstat_info->tab.counts_xact.delta_live_tuples += rec->tuples_inserted - rec->tuples_deleted; - pgstat_info->tab.counts.delta_dead_tuples += + pgstat_info->tab.counts_xact.delta_dead_tuples += rec->tuples_updated + rec->tuples_deleted; - pgstat_info->tab.counts.changed_tuples += + pgstat_info->tab.counts_xact.changed_tuples += rec->tuples_inserted + rec->tuples_updated + rec->tuples_deleted; } @@ -868,10 +868,10 @@ pgstat_twophase_postabort(FullTransactionId fxid, uint16 info, rec->tuples_updated = rec->updated_pre_truncdrop; rec->tuples_deleted = rec->deleted_pre_truncdrop; } - pgstat_info->tab.counts.tuples_inserted += rec->tuples_inserted; - pgstat_info->tab.counts.tuples_updated += rec->tuples_updated; - pgstat_info->tab.counts.tuples_deleted += rec->tuples_deleted; - pgstat_info->tab.counts.delta_dead_tuples += + pgstat_info->tab.counts_xact.tuples_inserted += rec->tuples_inserted; + pgstat_info->tab.counts_xact.tuples_updated += rec->tuples_updated; + pgstat_info->tab.counts_xact.tuples_deleted += rec->tuples_deleted; + pgstat_info->tab.counts_xact.delta_dead_tuples += rec->tuples_inserted + rec->tuples_updated; } @@ -897,9 +897,17 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) lstats = (PgStat_RelationStatus *) entry_ref->pending; shtabstats = (PgStatShared_Relation *) entry_ref->shared_stats; - /* ignore entries that didn't accumulate any actual counts */ - if (pg_memory_is_all_zeros(&lstats->tab.counts, - sizeof(struct PgStat_TableCounts))) + /* + * Ignore entries that didn't accumulate any actual counts. If the table + * was truncated, we still need to flush the entry to reset the live/dead + * counters and ins_since_vacuum even when no other counts were + * accumulated. + */ + if (!lstats->tab.truncdropped && + pg_memory_is_all_zeros(&lstats->tab.counts, + sizeof(struct PgStat_TableCounts)) && + pg_memory_is_all_zeros(&lstats->tab.counts_xact, + sizeof(struct PgStat_TableCountsXact))) return true; if (!pgstat_lock_entry(entry_ref, nowait)) @@ -918,25 +926,25 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) } tabentry->tuples_returned += lstats->tab.counts.tuples_returned; tabentry->tuples_fetched += lstats->tab.counts.tuples_fetched; - tabentry->tuples_inserted += lstats->tab.counts.tuples_inserted; - tabentry->tuples_updated += lstats->tab.counts.tuples_updated; - tabentry->tuples_deleted += lstats->tab.counts.tuples_deleted; - tabentry->tuples_hot_updated += lstats->tab.counts.tuples_hot_updated; - tabentry->tuples_newpage_updated += lstats->tab.counts.tuples_newpage_updated; + tabentry->tuples_inserted += lstats->tab.counts_xact.tuples_inserted; + tabentry->tuples_updated += lstats->tab.counts_xact.tuples_updated; + tabentry->tuples_deleted += lstats->tab.counts_xact.tuples_deleted; + tabentry->tuples_hot_updated += lstats->tab.counts_xact.tuples_hot_updated; + tabentry->tuples_newpage_updated += lstats->tab.counts_xact.tuples_newpage_updated; /* * If table was truncated/dropped, first reset the live/dead counters. */ - if (lstats->tab.counts.truncdropped) + if (lstats->tab.truncdropped) { tabentry->live_tuples = 0; tabentry->dead_tuples = 0; tabentry->ins_since_vacuum = 0; } - tabentry->live_tuples += lstats->tab.counts.delta_live_tuples; - tabentry->dead_tuples += lstats->tab.counts.delta_dead_tuples; - tabentry->mod_since_analyze += lstats->tab.counts.changed_tuples; + tabentry->live_tuples += lstats->tab.counts_xact.delta_live_tuples; + tabentry->dead_tuples += lstats->tab.counts_xact.delta_dead_tuples; + tabentry->mod_since_analyze += lstats->tab.counts_xact.changed_tuples; /* * Using tuples_inserted to update ins_since_vacuum does mean that we'll @@ -945,7 +953,7 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) * triggering for inserts more often than they maybe should, which is * probably not going to be common enough to be too concerned about here. */ - tabentry->ins_since_vacuum += lstats->tab.counts.tuples_inserted; + tabentry->ins_since_vacuum += lstats->tab.counts_xact.tuples_inserted; tabentry->blocks_fetched += lstats->tab.counts.blocks_fetched; tabentry->blocks_hit += lstats->tab.counts.blocks_hit; @@ -961,9 +969,9 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) dbentry = pgstat_prep_database_pending(dboid); dbentry->tuples_returned += lstats->tab.counts.tuples_returned; dbentry->tuples_fetched += lstats->tab.counts.tuples_fetched; - dbentry->tuples_inserted += lstats->tab.counts.tuples_inserted; - dbentry->tuples_updated += lstats->tab.counts.tuples_updated; - dbentry->tuples_deleted += lstats->tab.counts.tuples_deleted; + dbentry->tuples_inserted += lstats->tab.counts_xact.tuples_inserted; + dbentry->tuples_updated += lstats->tab.counts_xact.tuples_updated; + dbentry->tuples_deleted += lstats->tab.counts_xact.tuples_deleted; dbentry->blocks_fetched += lstats->tab.counts.blocks_fetched; dbentry->blocks_hit += lstats->tab.counts.blocks_hit; diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 0d47d745c18f..081cd006666b 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -1922,7 +1922,7 @@ pg_stat_get_slru(PG_FUNCTION_ARGS) return (Datum) 0; } -#define PG_STAT_GET_XACT_RELENTRY_INT64(stat) \ +#define PG_STAT_GET_XACT_RELENTRY_INT64(member, stat) \ Datum \ CppConcat(pg_stat_get_xact_,stat)(PG_FUNCTION_ARGS) \ { \ @@ -1934,40 +1934,40 @@ CppConcat(pg_stat_get_xact_,stat)(PG_FUNCTION_ARGS) \ relid)) == NULL) \ result = 0; \ else \ - result = (int64) (tabentry->tab.counts.stat); \ + result = (int64) (tabentry->tab.member.stat); \ \ PG_RETURN_INT64(result); \ } /* pg_stat_get_xact_numscans */ -PG_STAT_GET_XACT_RELENTRY_INT64(numscans) +PG_STAT_GET_XACT_RELENTRY_INT64(counts, numscans) /* pg_stat_get_xact_tuples_returned */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_returned) +PG_STAT_GET_XACT_RELENTRY_INT64(counts, tuples_returned) /* pg_stat_get_xact_tuples_fetched */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_fetched) +PG_STAT_GET_XACT_RELENTRY_INT64(counts, tuples_fetched) /* pg_stat_get_xact_tuples_hot_updated */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_hot_updated) +PG_STAT_GET_XACT_RELENTRY_INT64(counts_xact, tuples_hot_updated) /* pg_stat_get_xact_tuples_newpage_updated */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_newpage_updated) +PG_STAT_GET_XACT_RELENTRY_INT64(counts_xact, tuples_newpage_updated) /* pg_stat_get_xact_blocks_fetched */ -PG_STAT_GET_XACT_RELENTRY_INT64(blocks_fetched) +PG_STAT_GET_XACT_RELENTRY_INT64(counts, blocks_fetched) /* pg_stat_get_xact_blocks_hit */ -PG_STAT_GET_XACT_RELENTRY_INT64(blocks_hit) +PG_STAT_GET_XACT_RELENTRY_INT64(counts, blocks_hit) /* pg_stat_get_xact_tuples_inserted */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted) +PG_STAT_GET_XACT_RELENTRY_INT64(counts_xact, tuples_inserted) /* pg_stat_get_xact_tuples_updated */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated) +PG_STAT_GET_XACT_RELENTRY_INT64(counts_xact, tuples_updated) /* pg_stat_get_xact_tuples_deleted */ -PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted) +PG_STAT_GET_XACT_RELENTRY_INT64(counts_xact, tuples_deleted) /* * Accessor macro for in-transaction index stats. diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 1040a65bc14e..7aedaafab906 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2349,6 +2349,7 @@ PgStat_StatTabEntry PgStat_StatsFileOp PgStat_SubXactStatus PgStat_TableCounts +PgStat_TableCountsXact PgStat_TableXactStatus PgStat_WalCounters PgStat_WalStats -- 2.55.0
signature.asc
Description: PGP signature
