From a8391d40266eaff6be24a2feb38eb849d56c2c4f Mon Sep 17 00:00:00 2001 From: alterego655 <824662526@qq.com> Date: Fri, 27 Mar 2026 20:05:56 +0800 Subject: [PATCH v1] Fix pg_stat_xact_* views leaking stats across transaction boundaries The pg_stat_xact_user_tables and pg_stat_xact_user_functions views are supposed to report activity for the current transaction only. However, because backend-local pending stats entries are now kept alive across transactions for asynchronous flushing, these views returned the total accumulated pending counters instead of just the current transaction's contribution. Fix by introducing a per-entry "transaction baseline" that lazily snapshots the pending counters the first time each entry is touched in a new top-level transaction. The xact-scoped accessor functions then subtract the baseline, yielding only the current transaction's delta. The baseline is keyed by MyProc->vxid.lxid and established via a static inline helper, pgstat_ensure_xact_baseline(), called at every nontransactional counter-increment site. After the first call per entry per transaction, the check reduces to a single integer comparison. For function stats, a new PgStat_FunctionPending wrapper struct embeds the existing PgStat_FunctionCounts at offset 0, so the flush callback requires only a trivial cast change. --- src/backend/utils/activity/pgstat.c | 2 +- src/backend/utils/activity/pgstat_function.c | 47 +++++-- src/backend/utils/activity/pgstat_relation.c | 36 ++++++ src/bin/psql/meson.build | 1 + src/bin/psql/t/040_pgstat_xact.pl | 121 +++++++++++++++++++ src/include/pgstat.h | 51 ++++++++ 6 files changed, 249 insertions(+), 9 deletions(-) create mode 100644 src/bin/psql/t/040_pgstat_xact.pl diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index eb8ccbaa628..730d474c216 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -326,7 +326,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .shared_size = sizeof(PgStatShared_Function), .shared_data_off = offsetof(PgStatShared_Function, stats), .shared_data_len = sizeof(((PgStatShared_Function *) 0)->stats), - .pending_size = sizeof(PgStat_FunctionCounts), + .pending_size = sizeof(PgStat_FunctionPending), .flush_pending_cb = pgstat_function_flush_cb, .reset_timestamp_cb = pgstat_function_reset_timestamp_cb, diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/activity/pgstat_function.c index e6b84283c6c..a8f1354d1ee 100644 --- a/src/backend/utils/activity/pgstat_function.c +++ b/src/backend/utils/activity/pgstat_function.c @@ -18,6 +18,7 @@ #include "postgres.h" #include "fmgr.h" +#include "storage/proc.h" #include "utils/inval.h" #include "utils/pgstat_internal.h" #include "utils/syscache.h" @@ -73,8 +74,9 @@ pgstat_init_function_usage(FunctionCallInfo fcinfo, PgStat_FunctionCallUsage *fcu) { PgStat_EntryRef *entry_ref; - PgStat_FunctionCounts *pending; + PgStat_FunctionPending *fpending; bool created_entry; + LocalTransactionId curlxid = MyProc->vxid.lxid; if (pgstat_track_functions <= fcinfo->flinfo->fn_stats) { @@ -119,12 +121,19 @@ pgstat_init_function_usage(FunctionCallInfo fcinfo, } } - pending = entry_ref->pending; + fpending = (PgStat_FunctionPending *) entry_ref->pending; - fcu->fs = pending; + /* Establish baseline on first call in this top-level transaction */ + if (fpending->baseline_lxid != curlxid) + { + fpending->xact_baseline = fpending->counts; + fpending->baseline_lxid = curlxid; + } + + fcu->fs = &fpending->counts; /* save stats for this function, later used to compensate for recursion */ - fcu->save_f_total_time = pending->total_time; + fcu->save_f_total_time = fpending->counts.total_time; /* save current backend-wide total time */ fcu->save_total = total_func_time; @@ -192,10 +201,12 @@ pgstat_end_function_usage(PgStat_FunctionCallUsage *fcu, bool finalize) bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) { + PgStat_FunctionPending *fpending; PgStat_FunctionCounts *localent; PgStatShared_Function *shfuncent; - localent = (PgStat_FunctionCounts *) entry_ref->pending; + fpending = (PgStat_FunctionPending *) entry_ref->pending; + localent = &fpending->counts; shfuncent = (PgStatShared_Function *) entry_ref->shared_stats; /* localent always has non-zero content */ @@ -229,12 +240,32 @@ PgStat_FunctionCounts * find_funcstat_entry(Oid func_id) { PgStat_EntryRef *entry_ref; + PgStat_FunctionPending *fpending; + PgStat_FunctionCounts *result; entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id); - if (entry_ref) - return entry_ref->pending; - return NULL; + if (!entry_ref) + return NULL; + + fpending = (PgStat_FunctionPending *) entry_ref->pending; + + /* Not called in this transaction — preserve NULL semantics */ + if (fpending->baseline_lxid != MyProc->vxid.lxid) + return NULL; + + result = palloc(sizeof(PgStat_FunctionCounts)); + result->numcalls = fpending->counts.numcalls + - fpending->xact_baseline.numcalls; + INSTR_TIME_SET_ZERO(result->total_time); + INSTR_TIME_ACCUM_DIFF(result->total_time, + fpending->counts.total_time, + fpending->xact_baseline.total_time); + INSTR_TIME_SET_ZERO(result->self_time); + INSTR_TIME_ACCUM_DIFF(result->self_time, + fpending->counts.self_time, + fpending->xact_baseline.self_time); + return result; } /* diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index bc8c43b96aa..f814ea69797 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -20,6 +20,7 @@ #include "access/twophase_rmgr.h" #include "access/xact.h" #include "catalog/catalog.h" +#include "storage/proc.h" #include "utils/memutils.h" #include "utils/pgstat_internal.h" #include "utils/rel.h" @@ -376,6 +377,7 @@ pgstat_count_heap_insert(Relation rel, PgStat_Counter n) { PgStat_TableStatus *pgstat_info = rel->pgstat_info; + pgstat_ensure_xact_baseline(pgstat_info); ensure_tabstat_xact_level(pgstat_info); pgstat_info->trans->tuples_inserted += n; } @@ -393,6 +395,7 @@ pgstat_count_heap_update(Relation rel, bool hot, bool newpage) { PgStat_TableStatus *pgstat_info = rel->pgstat_info; + pgstat_ensure_xact_baseline(pgstat_info); ensure_tabstat_xact_level(pgstat_info); pgstat_info->trans->tuples_updated++; @@ -417,6 +420,7 @@ pgstat_count_heap_delete(Relation rel) { PgStat_TableStatus *pgstat_info = rel->pgstat_info; + pgstat_ensure_xact_baseline(pgstat_info); ensure_tabstat_xact_level(pgstat_info); pgstat_info->trans->tuples_deleted++; } @@ -432,6 +436,7 @@ pgstat_count_truncate(Relation rel) { PgStat_TableStatus *pgstat_info = rel->pgstat_info; + pgstat_ensure_xact_baseline(pgstat_info); ensure_tabstat_xact_level(pgstat_info); save_truncdrop_counters(pgstat_info->trans, false); pgstat_info->trans->tuples_inserted = 0; @@ -523,6 +528,33 @@ find_tabstat_entry(Oid rel_id) */ tablestatus->trans = NULL; + /* + * Adjust counters to show only the current top-level transaction's + * activity. If the baseline was set in the current transaction, subtract + * it to remove prior-transaction data. Otherwise, the entry was not + * touched in the current transaction, so all counters are zero. + */ + if (tabentry->baseline_lxid == MyProc->vxid.lxid) + { + PgStat_TableCounts *base = &tabentry->xact_baseline; + + tablestatus->counts.numscans -= base->numscans; + tablestatus->counts.tuples_returned -= base->tuples_returned; + tablestatus->counts.tuples_fetched -= base->tuples_fetched; + tablestatus->counts.tuples_inserted -= base->tuples_inserted; + tablestatus->counts.tuples_updated -= base->tuples_updated; + tablestatus->counts.tuples_deleted -= base->tuples_deleted; + tablestatus->counts.tuples_hot_updated -= base->tuples_hot_updated; + tablestatus->counts.tuples_newpage_updated -= base->tuples_newpage_updated; + tablestatus->counts.blocks_fetched -= base->blocks_fetched; + tablestatus->counts.blocks_hit -= base->blocks_hit; + } + else + { + /* Entry not touched in this transaction; all xact counters are zero */ + memset(&tablestatus->counts, 0, sizeof(PgStat_TableCounts)); + } + /* * Live subtransaction counts are not included yet. This is not a hot * code path so reconcile tuples_inserted, tuples_updated and @@ -752,6 +784,8 @@ pgstat_twophase_postcommit(FullTransactionId fxid, uint16 info, /* Find or create a tabstat entry for the rel */ pgstat_info = pgstat_prep_relation_pending(rec->id, rec->shared); + pgstat_ensure_xact_baseline(pgstat_info); + /* Same math as in AtEOXact_PgStat, commit case */ pgstat_info->counts.tuples_inserted += rec->tuples_inserted; pgstat_info->counts.tuples_updated += rec->tuples_updated; @@ -788,6 +822,8 @@ pgstat_twophase_postabort(FullTransactionId fxid, uint16 info, /* Find or create a tabstat entry for the rel */ pgstat_info = pgstat_prep_relation_pending(rec->id, rec->shared); + pgstat_ensure_xact_baseline(pgstat_info); + /* Same math as in AtEOXact_PgStat, abort case */ if (rec->truncdropped) { diff --git a/src/bin/psql/meson.build b/src/bin/psql/meson.build index 922b2845267..25fbb817f1d 100644 --- a/src/bin/psql/meson.build +++ b/src/bin/psql/meson.build @@ -78,6 +78,7 @@ tests += { 't/010_tab_completion.pl', 't/020_cancel.pl', 't/030_pager.pl', + 't/040_pgstat_xact.pl', ], }, } diff --git a/src/bin/psql/t/040_pgstat_xact.pl b/src/bin/psql/t/040_pgstat_xact.pl new file mode 100644 index 00000000000..d53a7424c4c --- /dev/null +++ b/src/bin/psql/t/040_pgstat_xact.pl @@ -0,0 +1,121 @@ +# Copyright (c) 2021-2026, PostgreSQL Global Development Group + +# Test that pg_stat_xact_* views report only current-transaction activity, +# not accumulated pending stats from prior transactions. +# +# This test uses "psql -c" to send multi-statement strings as a single +# simple-query protocol message. This ensures there is no ReadyForQuery +# boundary (and therefore no pgstat_report_stat() opportunity) between +# consecutive top-level transactions within the same message, which is the +# condition required to deterministically reproduce the cross-transaction +# stats leakage bug. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init; +$node->append_conf('postgresql.conf', "track_functions = 'all'"); +$node->start; + +my $dbname = 'postgres'; + +# Create test objects. +$node->safe_psql( + $dbname, q{ + CREATE TABLE test_xact_stats (x int); + CREATE FUNCTION test_xact_stats_func() RETURNS void + LANGUAGE plpgsql AS $$ BEGIN NULL; END; $$; +}); + +# Like psql_like in 001_basic.pl, but sends the SQL via "psql -c" so +# that all statements go as a single simple-query message. +sub psql_c_like +{ + local $Test::Builder::Level = $Test::Builder::Level + 1; + + my ($node, $sql, $expected_stdout, $test_name) = @_; + + my ($stdout, $stderr); + my @cmd = ( + $node->installed_command('psql'), + '--no-psqlrc', '--no-align', '--tuples-only', '--quiet', + '-d', $node->connstr($dbname), + '-c', $sql); + my $result = IPC::Run::run \@cmd, '>', \$stdout, '2>', \$stderr; + + is($result, 1, "$test_name: exit code 0"); + is($stderr, '', "$test_name: no stderr"); + like($stdout, $expected_stdout, "$test_name: matches"); + + return; +} + + +## +## Test 1: Table n_tup_ins must not leak across top-level transactions. +## +## Two consecutive transactions each insert one row. Without the fix, +## pg_stat_xact_user_tables in the second transaction would show +## n_tup_ins = 2 (accumulated) instead of 1. +## +psql_c_like( + $node, + q{BEGIN; + INSERT INTO test_xact_stats VALUES (1); + COMMIT; + BEGIN; + INSERT INTO test_xact_stats VALUES (2); + SELECT n_tup_ins FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/^1$/m, + "table n_tup_ins shows only current transaction's inserts"); + + +## +## Test 2: seq_scan counter must not leak across top-level transactions. +## +psql_c_like( + $node, + q{BEGIN; + SELECT count(*) FROM test_xact_stats; + COMMIT; + BEGIN; + SELECT count(*) FROM test_xact_stats; + SELECT seq_scan FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/1$/m, + "table seq_scan shows only current transaction's scans"); + + +## +## Test 3: Function call stats must not leak across top-level transactions. +## +psql_c_like( + $node, + q{BEGIN; + SELECT test_xact_stats_func(); + SELECT test_xact_stats_func(); + COMMIT; + BEGIN; + SELECT test_xact_stats_func(); + SELECT calls FROM pg_stat_xact_user_functions + WHERE funcname = 'test_xact_stats_func'; + COMMIT;}, + qr/^1$/m, + "function calls shows only current transaction's calls"); + +# Cleanup +$node->safe_psql( + $dbname, q{ + DROP TABLE test_xact_stats; + DROP FUNCTION test_xact_stats_func(); +}); + +$node->stop; +done_testing(); diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 8e3549c3752..e2776e7630c 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -16,6 +16,7 @@ #include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */ #include "replication/conflict.h" #include "storage/locktag.h" +#include "storage/proc.h" #include "utils/backend_progress.h" /* for backward compatibility */ /* IWYU pragma: export */ #include "utils/backend_status.h" /* for backward compatibility */ /* IWYU pragma: export */ #include "utils/pgstat_kind.h" @@ -90,6 +91,17 @@ typedef struct PgStat_FunctionCounts instr_time self_time; } PgStat_FunctionCounts; +/* + * Backend-local pending state for function stats, wrapping the accumulated + * counts with a per-transaction baseline for pg_stat_xact_* isolation. + */ +typedef struct PgStat_FunctionPending +{ + PgStat_FunctionCounts counts; /* accumulated counts */ + PgStat_FunctionCounts xact_baseline; /* snapshot at top-level txn start */ + LocalTransactionId baseline_lxid; /* lxid when baseline was set */ +} PgStat_FunctionPending; + /* * Working state needed to accumulate per-function-call timing statistics. */ @@ -183,6 +195,8 @@ typedef struct PgStat_TableStatus bool shared; /* is it a shared catalog? */ struct PgStat_TableXactStatus *trans; /* lowest subxact's counts */ PgStat_TableCounts counts; /* event counts to be sent */ + PgStat_TableCounts xact_baseline; /* snapshot at top-level txn start */ + LocalTransactionId baseline_lxid; /* lxid when baseline was set */ Relation relation; /* rel that is using this entry */ } PgStat_TableStatus; @@ -721,39 +735,76 @@ extern void pgstat_report_analyze(Relation rel, #define pgstat_count_heap_scan(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.numscans++; \ + } \ } while (0) #define pgstat_count_heap_getnext(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.tuples_returned++; \ + } \ } while (0) #define pgstat_count_heap_fetch(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.tuples_fetched++; \ + } \ } while (0) #define pgstat_count_index_scan(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.numscans++; \ + } \ } while (0) #define pgstat_count_index_tuples(rel, n) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.tuples_returned += (n); \ + } \ } while (0) #define pgstat_count_buffer_read(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.blocks_fetched++; \ + } \ } while (0) #define pgstat_count_buffer_hit(rel) \ do { \ if (pgstat_should_count_relation(rel)) \ + { \ + pgstat_ensure_xact_baseline((rel)->pgstat_info); \ (rel)->pgstat_info->counts.blocks_hit++; \ + } \ } while (0) +/* + * Ensure the xact baseline is current for this top-level transaction. + * Must be called before any counter in PgStat_TableStatus.counts is modified. + */ +static inline void +pgstat_ensure_xact_baseline(PgStat_TableStatus *pgstat_info) +{ + LocalTransactionId curlxid = MyProc->vxid.lxid; + + if (unlikely(pgstat_info->baseline_lxid != curlxid)) + { + pgstat_info->xact_baseline = pgstat_info->counts; + pgstat_info->baseline_lxid = curlxid; + } +} + extern void pgstat_count_heap_insert(Relation rel, PgStat_Counter n); extern void pgstat_count_heap_update(Relation rel, bool hot, bool newpage); extern void pgstat_count_heap_delete(Relation rel); -- 2.51.0