From 9f0eec731a75f52bd8f3e423c1ae22dea69b0a5a Mon Sep 17 00:00:00 2001 From: alterego655 <824662526@qq.com> Date: Mon, 30 Mar 2026 10:47:53 +0800 Subject: [PATCH v1] Fix pg_stat_xact_* views leaking stats across transaction boundaries Currently, pgStatPending entries persist across transaction boundaries within a backend. The pg_stat_xact_* view accessors (find_tabstat_entry, find_funcstat_entry) return raw accumulated counters from these entries, so they incorrectly report activity from prior transactions rather than only the current one. Fix this by recording a baseline snapshot of each pending entry's counters at every top-level transaction boundary. A new function, pgstat_set_pending_baselines(), sweeps the pgStatPending list at commit, abort, and PREPARE TRANSACTION, copying each entry's current counts into an xact_baseline field. The view accessors then subtract the baseline, yielding only the current transaction's delta. For entries first created in the current transaction, xact_baseline is zero-initialized, so the subtraction is a no-op. For function stats, introduce a PgStat_FunctionPending wrapper struct that pairs the existing PgStat_FunctionCounts with a baseline copy. find_funcstat_entry() now returns a palloc'd delta, or NULL when the function was not called in the current transaction. Add a TAP test under src/bin/psql/t/ that verifies counter isolation across consecutive transactions within a single simple-query message. --- src/backend/utils/activity/pgstat.c | 43 +++- src/backend/utils/activity/pgstat_function.c | 42 +++- src/backend/utils/activity/pgstat_relation.c | 21 ++ src/backend/utils/activity/pgstat_xact.c | 15 ++ src/bin/psql/meson.build | 1 + src/bin/psql/t/040_pgstat_xact.pl | 203 +++++++++++++++++++ src/include/pgstat.h | 16 ++ src/include/utils/pgstat_internal.h | 1 + 8 files changed, 332 insertions(+), 10 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..0d14e753bea 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, @@ -1361,6 +1361,47 @@ pgstat_delete_pending_entry(PgStat_EntryRef *entry_ref) dlist_delete(&entry_ref->pending_node); } +/* + * Set xact baselines for all pending relation and function entries. + * + * Called at top-level transaction commit, abort, and successful PREPARE + * to record the current counter state as the baseline. The next + * transaction on this backend will compute xact-scoped deltas by + * subtracting this baseline. + * + * This sweeps all unflushed pending entries, not just those touched in + * the current transaction, because nontransactional counters (scans, + * buffer hits, etc.) accumulate in the pending entry regardless of + * whether a PgStat_TableXactStatus was created. + */ +void +pgstat_set_pending_baselines(void) +{ + dlist_iter iter; + + dlist_foreach(iter, &pgStatPending) + { + PgStat_EntryRef *entry_ref = + dlist_container(PgStat_EntryRef, pending_node, iter.cur); + PgStat_Kind kind = entry_ref->shared_entry->key.kind; + + if (kind == PGSTAT_KIND_RELATION) + { + PgStat_TableStatus *tabstat = + (PgStat_TableStatus *) entry_ref->pending; + + tabstat->xact_baseline = tabstat->counts; + } + else if (kind == PGSTAT_KIND_FUNCTION) + { + PgStat_FunctionPending *fpending = + (PgStat_FunctionPending *) entry_ref->pending; + + fpending->xact_baseline = fpending->counts; + } + } +} + /* * Flush out pending variable-numbered stats. */ diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/activity/pgstat_function.c index e6b84283c6c..704cd1d8240 100644 --- a/src/backend/utils/activity/pgstat_function.c +++ b/src/backend/utils/activity/pgstat_function.c @@ -119,7 +119,7 @@ pgstat_init_function_usage(FunctionCallInfo fcinfo, } } - pending = entry_ref->pending; + pending = &((PgStat_FunctionPending *) entry_ref->pending)->counts; fcu->fs = pending; @@ -192,10 +192,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 */ @@ -223,18 +225,40 @@ pgstat_function_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts) /* * find any existing PgStat_FunctionCounts entry for specified function * - * If no entry, return NULL, don't create a new one + * Returns a palloc'd delta struct showing only the current transaction's + * contribution, or NULL if the function was not called in this transaction. */ PgStat_FunctionCounts * find_funcstat_entry(Oid func_id) { PgStat_EntryRef *entry_ref; - - entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id); - - if (entry_ref) - return entry_ref->pending; - return NULL; + PgStat_FunctionPending *fpending; + PgStat_FunctionCounts *result; + PgStat_Counter delta_calls; + + entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_FUNCTION, + MyDatabaseId, func_id); + if (!entry_ref) + return NULL; + + fpending = (PgStat_FunctionPending *) entry_ref->pending; + + /* Compute delta; return NULL if not called in this transaction */ + delta_calls = fpending->counts.numcalls - fpending->xact_baseline.numcalls; + if (delta_calls == 0) + return NULL; + + result = palloc(sizeof(PgStat_FunctionCounts)); + result->numcalls = delta_calls; + 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..4411870ba5e 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -503,6 +503,7 @@ find_tabstat_entry(Oid rel_id) PgStat_TableXactStatus *trans; PgStat_TableStatus *tabentry = NULL; PgStat_TableStatus *tablestatus = NULL; + PgStat_TableCounts *tabentry_baseline; entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id); if (!entry_ref) @@ -523,6 +524,26 @@ find_tabstat_entry(Oid rel_id) */ tablestatus->trans = NULL; + /* + * Subtract the xact baseline to show only the current top-level + * transaction's activity. The baseline is set at the end of each + * top-level transaction (commit/abort/prepare) by + * pgstat_set_pending_baselines(). For entries created in the current + * transaction, the baseline is zero-initialized, so subtracting it is a + * no-op. + */ + tabentry_baseline = &tabentry->xact_baseline; + tablestatus->counts.numscans -= tabentry_baseline->numscans; + tablestatus->counts.tuples_returned -= tabentry_baseline->tuples_returned; + tablestatus->counts.tuples_fetched -= tabentry_baseline->tuples_fetched; + tablestatus->counts.tuples_inserted -= tabentry_baseline->tuples_inserted; + tablestatus->counts.tuples_updated -= tabentry_baseline->tuples_updated; + tablestatus->counts.tuples_deleted -= tabentry_baseline->tuples_deleted; + tablestatus->counts.tuples_hot_updated -= tabentry_baseline->tuples_hot_updated; + tablestatus->counts.tuples_newpage_updated -= tabentry_baseline->tuples_newpage_updated; + tablestatus->counts.blocks_fetched -= tabentry_baseline->blocks_fetched; + tablestatus->counts.blocks_hit -= tabentry_baseline->blocks_hit; + /* * Live subtransaction counts are not included yet. This is not a hot * code path so reconcile tuples_inserted, tuples_updated and diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c index 5e2d69e6297..6a708beb05b 100644 --- a/src/backend/utils/activity/pgstat_xact.c +++ b/src/backend/utils/activity/pgstat_xact.c @@ -55,6 +55,14 @@ AtEOXact_PgStat(bool isCommit, bool parallel) } pgStatXactStack = NULL; + /* + * Record current counter values as the baseline for the next transaction. + * Must be after AtEOXact_PgStat_Relations() (which folds transactional + * counts into ->counts) and AtEOXact_PgStat_DroppedStats() (which removes + * dropped entries from pgStatPending). + */ + pgstat_set_pending_baselines(); + /* Make sure any stats snapshot is thrown away */ pgstat_clear_snapshot(); } @@ -226,6 +234,13 @@ PostPrepare_PgStat(void) } pgStatXactStack = NULL; + /* + * Record baselines, same as at commit/abort. AtEOXact_PgStat() is not + * called during PREPARE, so without this, nontransactional counters from + * the prepared transaction would leak into later transactions. + */ + pgstat_set_pending_baselines(); + /* Make sure any stats snapshot is thrown away */ pgstat_clear_snapshot(); } 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..cfb5cec3927 --- /dev/null +++ b/src/bin/psql/t/040_pgstat_xact.pl @@ -0,0 +1,203 @@ +# 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. +# +# Uses "psql -c" so all statements go as a single simple-query message, +# ensuring no ReadyForQuery boundary (and no pgstat_report_stat() flush) +# between consecutive top-level transactions. + +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->append_conf('postgresql.conf', "max_prepared_transactions = 5"); +$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; $$; +}); + +# Send SQL via "psql -c" so all statements are one 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. +## +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:' || n_tup_ins FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/^n_tup_ins:1$/m, + "table n_tup_ins shows only current transaction's inserts"); + +## +## Test 2: seq_scan counter must not leak across top-level transactions. +## Uses labeled output to avoid false-positive matches from count(*) rows. +## +psql_c_like( + $node, + q{BEGIN; + SELECT count(*) FROM test_xact_stats; + COMMIT; + BEGIN; + SELECT count(*) FROM test_xact_stats; + SELECT 'seq_scan:' || seq_scan FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/^seq_scan: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:' || calls FROM pg_stat_xact_user_functions + WHERE funcname = 'test_xact_stats_func'; + COMMIT;}, + qr/^calls:1$/m, + "function calls shows only current transaction's calls"); + +## +## Test 4: seq_scan must not leak across a PREPARE TRANSACTION boundary. +## +## COMMIT PREPARED cannot appear in a multi-statement psql -c string. +## In a multi-statement simple-query message, exec_simple_query() calls +## BeginImplicitTransactionBlock() before each statement. After PREPARE +## TRANSACTION ends the explicit block, the loop re-enters the implicit- +## block path, putting the backend into TBLOCK_IMPLICIT_INPROGRESS. +## PreventInTransactionBlock() rejects COMMIT PREPARED in that state. +## +## A separate psql() call works because it sends a single-statement +## simple-query message: use_implicit_block is false, so the backend +## stays in TBLOCK_STARTED and PreventInTransactionBlock() passes. +## +psql_c_like( + $node, + q{BEGIN; + SELECT count(*) FROM test_xact_stats; + PREPARE TRANSACTION 'test_prep'; + BEGIN; + SELECT count(*) FROM test_xact_stats; + SELECT 'seq_scan:' || seq_scan FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/^seq_scan:1$/m, + "table seq_scan doesn't leak across PREPARE boundary"); + +# Best-effort cleanup: use psql() instead of safe_psql() so that if the +# preceding test failed before PREPARE TRANSACTION, the missing prepared +# xact does not abort the rest of the test file. +$node->psql($dbname, q{COMMIT PREPARED 'test_prep';}); + +## +## Test 5: Untouched relation entry must show zero, not prior-txn counts. +## +## If a pending entry exists from a prior transaction but the current +## transaction has not touched the relation, counts - baseline = 0. +## +psql_c_like( + $node, + q{BEGIN; + SELECT count(*) FROM test_xact_stats; + INSERT INTO test_xact_stats VALUES (99); + COMMIT; + BEGIN; + SELECT 'seq_scan:' || seq_scan || ',n_tup_ins:' || n_tup_ins + FROM pg_stat_xact_user_tables + WHERE relname = 'test_xact_stats'; + COMMIT;}, + qr/^seq_scan:0,n_tup_ins:0$/m, + "untouched relation entry shows zeroes in current transaction"); + +## +## Test 6: Untouched function entry must not appear in xact view. +## +## pg_stat_xact_user_functions filters with IS NOT NULL on the accessor. +## When delta_calls == 0, find_funcstat_entry() returns NULL, so the +## function must not appear. +## +psql_c_like( + $node, + q{BEGIN; + SELECT test_xact_stats_func(); + COMMIT; + BEGIN; + SELECT 'found:' || count(*) FROM pg_stat_xact_user_functions + WHERE funcname = 'test_xact_stats_func'; + COMMIT;}, + qr/^found:0$/m, + "uncalled function not visible in pg_stat_xact_user_functions"); + +## +## Test 7: Function call stats must not leak across PREPARE boundary. +## +psql_c_like( + $node, + q{BEGIN; + SELECT test_xact_stats_func(); + SELECT test_xact_stats_func(); + PREPARE TRANSACTION 'test_func_prep'; + BEGIN; + SELECT test_xact_stats_func(); + SELECT 'calls:' || calls FROM pg_stat_xact_user_functions + WHERE funcname = 'test_xact_stats_func'; + COMMIT;}, + qr/^calls:1$/m, + "function calls don't leak across PREPARE boundary"); + +# Best-effort cleanup (see comment above). +$node->psql($dbname, q{COMMIT PREPARED 'test_func_prep';}); + +# 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..660b64e870a 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -90,6 +90,21 @@ typedef struct PgStat_FunctionCounts instr_time self_time; } PgStat_FunctionCounts; +/* ---------- + * PgStat_FunctionPending Backend-local pending state for function stats + * + * Wraps the accumulated counts with a per-transaction baseline so that + * pg_stat_xact_user_functions can report only the current transaction's + * contribution. The baseline is set at each top-level transaction boundary + * by pgstat_set_pending_baselines(). + * ---------- + */ +typedef struct PgStat_FunctionPending +{ + PgStat_FunctionCounts counts; /* accumulated counts */ + PgStat_FunctionCounts xact_baseline; /* snapshot at txn boundary */ +} PgStat_FunctionPending; + /* * Working state needed to accumulate per-function-call timing statistics. */ @@ -183,6 +198,7 @@ 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 txn boundary */ Relation relation; /* rel that is using this entry */ } PgStat_TableStatus; diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index 97704421a92..292d9757c02 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -687,6 +687,7 @@ extern PgStat_EntryRef *pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid, bool *created_entry); extern PgStat_EntryRef *pgstat_fetch_pending_entry(PgStat_Kind kind, Oid dboid, uint64 objid); +extern void pgstat_set_pending_baselines(void); extern void *pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid); extern void pgstat_snapshot_fixed(PgStat_Kind kind); -- 2.51.0