pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedxact_rollback spikes when logical walsender exits
8+ messages / 5 participants
[nested] [flat]
* xact_rollback spikes when logical walsender exits
@ 2026-04-17 15:15 Nikolay Samokhvalov <nik@postgres.ai>
0 siblings, 2 replies; 8+ messages in thread
From: Nikolay Samokhvalov @ 2026-04-17 15:15 UTC (permalink / raw)
To: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Rafael Thofehrn Castro <rafaelthca@gmail.com>
Hi hackers,
There is a bug on logical-replication publishers where every decoded
committed transaction bumps pg_stat_database.xact_rollback.
ReorderBufferProcessTXN() ends each decoded transaction with
AbortCurrentTransaction() for catalog cleanup; in the walsender that
is a top-level abort, so AtEOXact_PgStat_Database(isCommit=false)
increments the backend-local pgStatXactRollback.
The counts are flushed to shared stats on walsender exit, producing
an acute spike. Result: for production systems with SREs on call and tight
alerting on xact_rollback, this turns routine logical-replication operations
(disabling a subscription, dropping a slot, walsender restart) into
false-positive pages.
Reported in [1]; also experienced at GitLab [2][3][4].
Attaching a simple patch that adds a backend-local flag pgStatXactSkipCounters
in pgstat_database.c that AtEOXact_PgStat_Database() honors to skip
the counter bump.
Added TAP test that fails on master with 5/0 and passes with the patch.
If there is agreement on this shape, happy to send patches for all
supported branches. Let me know what you think.
[1] https://postgr.es/m/CAG0ozMo_xWQn%2BAvv8jzbbhePGp5OnhdO%2BYWTkdg4faWSXz0Jzg%40mail.gmail.com
[2] https://gitlab.com/gitlab-com/gl-infra/production/-/work_items/8290
[3] https://gitlab.com/postgres-ai/postgresql-consulting/tests-and-benchmarks/-/work_items/39
[4] https://gitlab.com/gitlab-org/orbit/knowledge-graph/-/work_items/406
Nik
Attachments:
v1-logical-rollback-spike.patch (application/octet-stream, 9.7K) download
From 11b70a070c40433389fec476464078a7038384e3 Mon Sep 17 00:00:00 2001
From: Nik Samokhvalov <nik@niks-mbp.lan>
Date: Thu, 16 Apr 2026 19:25:55 -0700
Subject: [PATCH] Do not attribute logical decoding aborts to xact_rollback
On a logical-replication publisher, every decoded committed
transaction bumps pg_stat_database.xact_rollback because the walsender
ends each decoded transaction with a top-level AbortCurrentTransaction()
for catalog cleanup. The counts surface in shared stats on walsender
exit, producing a visible spike that has nothing to do with user
rollbacks.
Add pgStatXactSkipCounters, a backend-local flag consulted by
AtEOXact_PgStat_Database() alongside the existing parallel-worker
check. ReorderBufferProcessTXN() (both abort sites, gated on
!using_subtxn) and SnapBuildClearExportedSnapshot() bracket their
AbortCurrentTransaction() with it. StartTransaction() asserts the
flag is clear to catch leaks.
TAP test src/test/subscription/t/039_publisher_xact_rollback.pl:
5 autocommit INSERTs, DISABLE subscription, assert the xact_rollback
delta is 0.
Reported-by: Rafael Thofehrn Castro
Discussion: https://postgr.es/m/CAG0ozMo_xWQn%2BAvv8jzbbhePGp5OnhdO%2BYWTkdg4faWSXz0Jzg%40mail.gmail.com
---
src/backend/access/transam/xact.c | 1 +
.../replication/logical/reorderbuffer.c | 14 +++-
src/backend/replication/logical/snapbuild.c | 9 ++-
src/backend/utils/activity/pgstat_database.c | 31 ++++----
src/include/pgstat.h | 3 +
src/test/subscription/meson.build | 1 +
.../t/039_publisher_xact_rollback.pl | 71 +++++++++++++++++++
7 files changed, 115 insertions(+), 15 deletions(-)
create mode 100644 src/test/subscription/t/039_publisher_xact_rollback.pl
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 48bc90c967353..18dff512000c7 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2118,6 +2118,7 @@ StartTransaction(void)
/* check the current transaction state */
Assert(s->state == TRANS_DEFAULT);
+ Assert(!pgStatXactSkipCounters);
/*
* Set the current transaction state information appropriately during
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 682d13c9f22f0..c100506e259bd 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2674,9 +2674,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
* Aborting the current (sub-)transaction as a whole has the right
* semantics. We want all locks acquired in here to be released, not
* reassigned to the parent and we do not want any database access
- * have persistent effects.
+ * have persistent effects. In the !using_subtxn case this is a
+ * top-level abort; keep it out of pg_stat_database.xact_rollback.
*/
+ if (!using_subtxn)
+ pgStatXactSkipCounters = true;
AbortCurrentTransaction();
+ /* unconditional clear is fine; it was false coming in */
+ pgStatXactSkipCounters = false;
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
@@ -2737,9 +2742,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/*
* Force cache invalidation to happen outside of a valid transaction
- * to prevent catalog access as we just caught an error.
+ * to prevent catalog access as we just caught an error. As above,
+ * keep the top-level abort out of pg_stat_database.xact_rollback.
*/
+ if (!using_subtxn)
+ pgStatXactSkipCounters = true;
AbortCurrentTransaction();
+ /* unconditional clear is fine; it was false coming in */
+ pgStatXactSkipCounters = false;
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index c8309b96ed45c..d15db19abf3b6 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -631,8 +631,15 @@ SnapBuildClearExportedSnapshot(void)
*/
tmpResOwner = SavedResourceOwnerDuringExport;
- /* make sure nothing could have ever happened */
+ /*
+ * Make sure nothing could have ever happened. Keep this cleanup abort
+ * out of pg_stat_database.xact_rollback; the invariant that we are at
+ * top level is required so the abort reaches AtEOXact_PgStat_Database.
+ */
+ Assert(!IsSubTransaction());
+ pgStatXactSkipCounters = true;
AbortCurrentTransaction();
+ pgStatXactSkipCounters = false;
CurrentResourceOwner = tmpResOwner;
}
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 7f3bc0165931c..00c349631ca7b 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -291,21 +291,28 @@ pgstat_fetch_stat_dbentry(Oid dboid)
pgstat_fetch_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid, NULL);
}
+/*
+ * When true, AtEOXact_PgStat_Database() skips the xact_commit /
+ * xact_rollback bump for the current (sub-)transaction end. Logical
+ * decoding sets this around its catalog-cleanup AbortCurrentTransaction()
+ * so that abort is not attributed to pg_stat_database.
+ */
+bool pgStatXactSkipCounters = false;
+
void
AtEOXact_PgStat_Database(bool isCommit, bool parallel)
{
- /* Don't count parallel worker transaction stats */
- if (!parallel)
- {
- /*
- * Count transaction commit or abort. (We use counters, not just
- * bools, in case the reporting message isn't sent right away.)
- */
- if (isCommit)
- pgStatXactCommit++;
- else
- pgStatXactRollback++;
- }
+ if (parallel || pgStatXactSkipCounters)
+ return;
+
+ /*
+ * Count transaction commit or abort. (We use counters, not just bools,
+ * in case the reporting message isn't sent right away.)
+ */
+ if (isCommit)
+ pgStatXactCommit++;
+ else
+ pgStatXactRollback++;
}
/*
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index dfa2e8376382a..8e90903b56b7f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -875,6 +875,9 @@ extern PGDLLIMPORT PgStat_Counter pgStatBlockWriteTime;
extern PGDLLIMPORT PgStat_Counter pgStatActiveTime;
extern PGDLLIMPORT PgStat_Counter pgStatTransactionIdleTime;
+/* see AtEOXact_PgStat_Database() */
+extern PGDLLIMPORT bool pgStatXactSkipCounters;
+
/* updated by the traffic cop and in errfinish() */
extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build
index e71e95c6297eb..268fa8c3e9c13 100644
--- a/src/test/subscription/meson.build
+++ b/src/test/subscription/meson.build
@@ -48,6 +48,7 @@ tests += {
't/036_sequences.pl',
't/037_except.pl',
't/038_walsnd_shutdown_timeout.pl',
+ 't/039_publisher_xact_rollback.pl',
't/100_bugs.pl',
],
},
diff --git a/src/test/subscription/t/039_publisher_xact_rollback.pl b/src/test/subscription/t/039_publisher_xact_rollback.pl
new file mode 100644
index 0000000000000..b7dc5b20d6176
--- /dev/null
+++ b/src/test/subscription/t/039_publisher_xact_rollback.pl
@@ -0,0 +1,71 @@
+
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Check that pg_stat_database.xact_rollback on a logical-replication
+# publisher is not inflated by the walsender's internal catalog-cleanup
+# aborts. ReorderBufferProcessTXN() ends each decoded transaction with
+# AbortCurrentTransaction(); in the walsender that is a top-level abort
+# whose counter increment flushes to shared stats on walsender exit.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+$node_subscriber->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+
+$node_publisher->safe_psql('postgres', 'CREATE PUBLICATION p FOR TABLE t');
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION s CONNECTION '$publisher_connstr' PUBLICATION p");
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 's');
+
+# Use a baseline-delta rather than pg_stat_reset() to tolerate ambient
+# rollback activity.
+my $base = $node_publisher->safe_psql('postgres',
+ "SELECT xact_rollback FROM pg_stat_database WHERE datname = 'postgres'");
+chomp $base;
+
+# Five autocommit INSERTs: each becomes one decoded committed txn on the
+# walsender. Without the fix, that's five spurious rollbacks after DISABLE.
+my $n = 5;
+$node_publisher->safe_psql('postgres',
+ join('', map { "INSERT INTO t VALUES ($_);\n" } 1 .. $n));
+
+$node_publisher->wait_for_catchup('s');
+
+# Disabling the subscription terminates the walsender; its shutdown hook
+# flushes pgstat counters to shared stats.
+$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE');
+
+# Wait for this subscription's walsender (filter by application_name).
+$node_publisher->poll_query_until(
+ 'postgres', q{
+ SELECT count(*) = 0 FROM pg_stat_activity
+ WHERE backend_type = 'walsender' AND application_name = 's'
+})
+ or die 's walsender did not exit';
+
+my $final = $node_publisher->safe_psql('postgres',
+ "SELECT xact_rollback FROM pg_stat_database WHERE datname = 'postgres'");
+chomp $final;
+
+cmp_ok(
+ $final - $base, '==', 0,
+ 'walsender does not inflate publisher xact_rollback for decoded transactions'
+);
+
+done_testing();
* Re: xact_rollback spikes when logical walsender exits
@ 2026-04-20 16:35 Fujii Masao <masao.fujii@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 1 reply; 8+ messages in thread
From: Fujii Masao @ 2026-04-20 16:35 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
On Sat, Apr 18, 2026 at 12:15 AM Nikolay Samokhvalov <nik@postgres.ai> wrote:
>
> Hi hackers,
>
> There is a bug on logical-replication publishers where every decoded
> committed transaction bumps pg_stat_database.xact_rollback.
> ReorderBufferProcessTXN() ends each decoded transaction with
> AbortCurrentTransaction() for catalog cleanup; in the walsender that
> is a top-level abort, so AtEOXact_PgStat_Database(isCommit=false)
> increments the backend-local pgStatXactRollback.
>
> The counts are flushed to shared stats on walsender exit, producing
> an acute spike. Result: for production systems with SREs on call and tight
> alerting on xact_rollback, this turns routine logical-replication operations
> (disabling a subscription, dropping a slot, walsender restart) into
> false-positive pages.
>
> Reported in [1]; also experienced at GitLab [2][3][4].
>
> Attaching a simple patch that adds a backend-local flag pgStatXactSkipCounters
> in pgstat_database.c that AtEOXact_PgStat_Database() honors to skip
> the counter bump.
>
> Added TAP test that fails on master with 5/0 and passes with the patch.
>
> If there is agreement on this shape, happy to send patches for all
> supported branches. Let me know what you think.
Thanks for the report and patch!
How to implement a solution depends on what xact_rollback in pg_stat_database
is intended to mean. So at first we should consider which rollbacks should
it count? The documentation does not currently give an explicit definition.
At present, xact_rollback appears to count all rollbacks, explicit or implicit,
by any process connected to the database, including regular backends,
autovacuum workers, and logical walsenders. If that is the intended definition,
then rollbacks implicitly performed by logical walsenders during logical
replication should also be counted. Of course, even if we keep that definition,
the sudden increase in xact_rollback might still be a problem, so we might
need to call pgstat_report_stat() immediately after pgstat_flush_io() in
walsender, so the counters continue to be updated periodically during
logical replication.
On the other hand, your patch seems to assume a different definition: that
xact_rollback should count all explicit and implicit rollbacks, except those
performed by logical walsenders during logical replication. That would be
one possible approach, although it seems a bit odd to exclude only one subset
of rollbacks.
A third option would be to define xact_rollback more narrowly, counting only
rollbacks by regular backends, and excluding rollbacks by processes such as
autovacuum or walsender. At least in my view, xact_commit and xact_rollback
in pg_stat_database are typically used by DBAs to check whether
client transactions are committing or rolling back as expected. From
that perspective, it seems intuitive for xact_rollback to count only rollbacks
by regular backends. But others may reasonably see it differently.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
@ 2026-04-21 06:38 vignesh C <vignesh21@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 1 reply; 8+ messages in thread
From: vignesh C @ 2026-04-21 06:38 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
On Fri, 17 Apr 2026 at 20:45, Nikolay Samokhvalov <nik@postgres.ai> wrote:
>
> Hi hackers,
>
> There is a bug on logical-replication publishers where every decoded
> committed transaction bumps pg_stat_database.xact_rollback.
> ReorderBufferProcessTXN() ends each decoded transaction with
> AbortCurrentTransaction() for catalog cleanup; in the walsender that
> is a top-level abort, so AtEOXact_PgStat_Database(isCommit=false)
> increments the backend-local pgStatXactRollback.
>
> The counts are flushed to shared stats on walsender exit, producing
> an acute spike. Result: for production systems with SREs on call and tight
> alerting on xact_rollback, this turns routine logical-replication operations
> (disabling a subscription, dropping a slot, walsender restart) into
> false-positive pages.
>
> Reported in [1]; also experienced at GitLab [2][3][4].
>
> Attaching a simple patch that adds a backend-local flag pgStatXactSkipCounters
> in pgstat_database.c that AtEOXact_PgStat_Database() honors to skip
> the counter bump.
>
> Added TAP test that fails on master with 5/0 and passes with the patch.
>
> If there is agreement on this shape, happy to send patches for all
> supported branches. Let me know what you think.
Thanks for reporting this and for the patch the problem description
matches what I've observed as well. The current behavior could be
misleading, since these rollbacks correspond to internal decoding
cleanup rather than actual user visible transaction aborts.
Another approach could be to introduce a wrapper around
AbortCurrentTransaction(), for example
AbortCurrentTransactionWithoutUpdateStats(), that skips the
AtEOXact_PgStat() call in this case.
Thoughts?
Regards,
Vignesh
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
@ 2026-05-13 00:06 Nikolay Samokhvalov <nik@postgres.ai>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Samokhvalov @ 2026-05-13 00:06 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
On Mon, Apr 20, 2026 at 11:38 PM vignesh C <vignesh21@gmail.com> wrote:
> Another approach could be to introduce a wrapper around
> AbortCurrentTransaction(), for example
> AbortCurrentTransactionWithoutUpdateStats(), that skips the
> AtEOXact_PgStat() call in this case.
> Thoughts?
Thanks -- v2 attached, adopting your wrapper.
One scope choice worth flagging: the wrapper suppresses only
AtEOXact_PgStat_Database() (the DB-level xact_commit/xact_rollback
counter), not all of AtEOXact_PgStat(). Per-relation and subxact stat
handling still run, so nothing accumulated during the cleanup is lost.
Renamed to AbortCurrentTransactionWithoutXactStats() to match.
Nik
Attachments:
v2-logical-rollback-spike.patch (application/octet-stream, 12.3K) download
From 62b52d0011f08d472f3d60b67110453b07f1017d Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <nik@postgres.ai>
Date: Thu, 16 Apr 2026 19:25:55 -0700
Subject: [PATCH] Do not count logical decoding cleanup aborts in xact_rollback
ReorderBufferProcessTXN() aborts the current transaction after each
decoded commit to release locks and clean up catalog access. In a
logical walsender that is a top-level abort, so every decoded commit
bumps pg_stat_database.xact_rollback; the counts surface as a spike on
walsender exit (e.g. when a subscription is disabled).
Add AbortCurrentTransactionWithoutXactStats(), a wrapper that suppresses
only the DB-level xact_commit/xact_rollback counter and leaves
per-relation and subxact stat handling intact. Use it from the
top-level cleanup paths in ReorderBufferProcessTXN() (gated on
!using_subtxn) and SnapBuildClearExportedSnapshot().
A TAP test asserts a publisher xact_rollback delta of 0 across five
decoded transactions (delta is 5 without the fix).
Reported-by: Rafael Thofehrn Castro
Discussion: https://postgr.es/m/CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb%3DQ2KzTw%40mail.gmail.com
---
src/backend/access/transam/twophase.c | 2 +-
src/backend/access/transam/xact.c | 39 ++++++++++-
.../replication/logical/reorderbuffer.c | 16 +++--
src/backend/replication/logical/snapbuild.c | 9 ++-
src/backend/utils/activity/pgstat_xact.c | 11 ++-
src/include/access/xact.h | 1 +
src/include/pgstat.h | 2 +-
src/test/subscription/meson.build | 1 +
.../t/039_publisher_xact_rollback.pl | 70 +++++++++++++++++++
9 files changed, 139 insertions(+), 12 deletions(-)
create mode 100644 src/test/subscription/t/039_publisher_xact_rollback.pl
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 1035e8b3fc7..3aa31e7f805 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1676,7 +1676,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
LWLockRelease(TwoPhaseStateLock);
/* Count the prepared xact as committed or aborted */
- AtEOXact_PgStat(isCommit, false);
+ AtEOXact_PgStat(isCommit, false, true);
/*
* And now we can clean up any files we may have left.
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 48bc90c9673..35d5e174011 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -316,6 +316,15 @@ typedef struct XactCallbackItem
static XactCallbackItem *Xact_callbacks = NULL;
+/*
+ * When true, suppress the pg_stat_database xact_commit/xact_rollback bump
+ * for the current transaction end. Must only be set via
+ * AbortCurrentTransactionWithoutXactStats(); assertions in
+ * StartTransaction() and in the wrapper itself guard against the flag
+ * leaking across transactions.
+ */
+static bool xactSkipXactStats = false;
+
/*
* List of add-on start- and end-of-subxact callbacks
*/
@@ -2118,6 +2127,7 @@ StartTransaction(void)
/* check the current transaction state */
Assert(s->state == TRANS_DEFAULT);
+ Assert(!xactSkipXactStats);
/*
* Set the current transaction state information appropriately during
@@ -2514,7 +2524,7 @@ CommitTransaction(void)
AtEOXact_Files(true);
AtEOXact_ComboCid();
AtEOXact_HashTables(true);
- AtEOXact_PgStat(true, is_parallel_worker);
+ AtEOXact_PgStat(true, is_parallel_worker, true);
AtEOXact_Snapshot(true, false);
AtEOXact_ApplyLauncher(true);
AtEOXact_LogicalRepWorkers(true);
@@ -3039,7 +3049,7 @@ AbortTransaction(void)
AtEOXact_Files(false);
AtEOXact_ComboCid();
AtEOXact_HashTables(false);
- AtEOXact_PgStat(false, is_parallel_worker);
+ AtEOXact_PgStat(false, is_parallel_worker, !xactSkipXactStats);
AtEOXact_ApplyLauncher(false);
AtEOXact_LogicalRepWorkers(false);
AtEOXact_LogicalCtl();
@@ -3509,6 +3519,31 @@ AbortCurrentTransaction(void)
}
}
+/*
+ * AbortCurrentTransactionWithoutXactStats
+ *
+ * Like AbortCurrentTransaction(), but do not count the transaction abort in
+ * pg_stat_database.xact_rollback. This is for internal cleanup aborts that
+ * release transaction-local resources but do not represent a user-visible
+ * transaction rollback.
+ */
+void
+AbortCurrentTransactionWithoutXactStats(void)
+{
+ Assert(!xactSkipXactStats);
+
+ xactSkipXactStats = true;
+ PG_TRY();
+ {
+ AbortCurrentTransaction();
+ }
+ PG_FINALLY();
+ {
+ xactSkipXactStats = false;
+ }
+ PG_END_TRY();
+}
+
/*
* AbortCurrentTransactionInternal - a function doing an iteration of work
* regarding handling the current transaction abort. In the case of
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 682d13c9f22..f953676bfe1 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2674,9 +2674,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
* Aborting the current (sub-)transaction as a whole has the right
* semantics. We want all locks acquired in here to be released, not
* reassigned to the parent and we do not want any database access
- * have persistent effects.
+ * have persistent effects. In the !using_subtxn case this is a
+ * top-level abort; keep it out of pg_stat_database.xact_rollback.
*/
- AbortCurrentTransaction();
+ if (using_subtxn)
+ AbortCurrentTransaction();
+ else
+ AbortCurrentTransactionWithoutXactStats();
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
@@ -2737,9 +2741,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/*
* Force cache invalidation to happen outside of a valid transaction
- * to prevent catalog access as we just caught an error.
+ * to prevent catalog access as we just caught an error. As above,
+ * keep the top-level abort out of pg_stat_database.xact_rollback.
*/
- AbortCurrentTransaction();
+ if (using_subtxn)
+ AbortCurrentTransaction();
+ else
+ AbortCurrentTransactionWithoutXactStats();
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index c8309b96ed4..ba9af88c505 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -631,8 +631,13 @@ SnapBuildClearExportedSnapshot(void)
*/
tmpResOwner = SavedResourceOwnerDuringExport;
- /* make sure nothing could have ever happened */
- AbortCurrentTransaction();
+ /*
+ * Make sure nothing could have ever happened. Keep this cleanup abort
+ * out of pg_stat_database.xact_rollback; we must be at top level so
+ * the abort reaches AtEOXact_PgStat_Database.
+ */
+ Assert(!IsSubTransaction());
+ AbortCurrentTransactionWithoutXactStats();
CurrentResourceOwner = tmpResOwner;
}
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c
index 5e2d69e6297..ea9f703c088 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -37,11 +37,18 @@ static PgStat_SubXactStatus *pgStatXactStack = NULL;
* Called from access/transam/xact.c at top-level transaction commit/abort.
*/
void
-AtEOXact_PgStat(bool isCommit, bool parallel)
+AtEOXact_PgStat(bool isCommit, bool parallel, bool count_xact_stats)
{
PgStat_SubXactStatus *xact_state;
- AtEOXact_PgStat_Database(isCommit, parallel);
+ /*
+ * Only the database-level xact_commit/xact_rollback counter is gated
+ * here. Per-relation and subxact stat handling below must still run
+ * unconditionally so any stats accumulated during the transaction are
+ * not lost.
+ */
+ if (count_xact_stats)
+ AtEOXact_PgStat_Database(isCommit, parallel);
/* handle transactional stats information */
xact_state = pgStatXactStack;
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index a8cbdf247c8..a5ec9a027d4 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -467,6 +467,7 @@ extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s);
extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s);
extern void CommitTransactionCommand(void);
extern void AbortCurrentTransaction(void);
+extern void AbortCurrentTransactionWithoutXactStats(void);
extern void BeginTransactionBlock(void);
extern bool EndTransactionBlock(bool chain);
extern bool PrepareTransactionBlock(const char *gid);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index dfa2e837638..8509a590bbf 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -814,7 +814,7 @@ extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid);
* Functions in pgstat_xact.c
*/
-extern void AtEOXact_PgStat(bool isCommit, bool parallel);
+extern void AtEOXact_PgStat(bool isCommit, bool parallel, bool count_xact_stats);
extern void AtEOSubXact_PgStat(bool isCommit, int nestDepth);
extern void AtPrepare_PgStat(void);
extern void PostPrepare_PgStat(void);
diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build
index e71e95c6297..268fa8c3e9c 100644
--- a/src/test/subscription/meson.build
+++ b/src/test/subscription/meson.build
@@ -48,6 +48,7 @@ tests += {
't/036_sequences.pl',
't/037_except.pl',
't/038_walsnd_shutdown_timeout.pl',
+ 't/039_publisher_xact_rollback.pl',
't/100_bugs.pl',
],
},
diff --git a/src/test/subscription/t/039_publisher_xact_rollback.pl b/src/test/subscription/t/039_publisher_xact_rollback.pl
new file mode 100644
index 00000000000..1fa72a19712
--- /dev/null
+++ b/src/test/subscription/t/039_publisher_xact_rollback.pl
@@ -0,0 +1,70 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Check that pg_stat_database.xact_rollback on a logical-replication
+# publisher is not inflated by the walsender's internal catalog-cleanup
+# aborts. ReorderBufferProcessTXN() ends each decoded transaction with
+# AbortCurrentTransaction(); in the walsender that is a top-level abort
+# whose counter increment flushes to shared stats on walsender exit.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+$node_subscriber->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+
+$node_publisher->safe_psql('postgres', 'CREATE PUBLICATION p FOR TABLE t');
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION s CONNECTION '$publisher_connstr' PUBLICATION p");
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 's');
+
+# Use a baseline-delta rather than pg_stat_reset() to tolerate ambient
+# rollback activity.
+my $base = $node_publisher->safe_psql('postgres',
+ "SELECT xact_rollback FROM pg_stat_database WHERE datname = 'postgres'");
+chomp $base;
+
+# Five autocommit INSERTs: each becomes one decoded committed txn on the
+# walsender. Without the fix, that's five spurious rollbacks after DISABLE.
+my $n = 5;
+$node_publisher->safe_psql('postgres',
+ join('', map { "INSERT INTO t VALUES ($_);\n" } 1 .. $n));
+
+$node_publisher->wait_for_catchup('s');
+
+# Disabling the subscription terminates the walsender; its shutdown hook
+# flushes pgstat counters to shared stats.
+$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE');
+
+# Wait for this subscription's walsender (filter by application_name).
+$node_publisher->poll_query_until(
+ 'postgres', q{
+ SELECT count(*) = 0 FROM pg_stat_activity
+ WHERE backend_type = 'walsender' AND application_name = 's'
+})
+ or die 's walsender did not exit';
+
+my $final = $node_publisher->safe_psql('postgres',
+ "SELECT xact_rollback FROM pg_stat_database WHERE datname = 'postgres'");
+chomp $final;
+
+cmp_ok(
+ $final - $base, '==', 0,
+ 'walsender does not inflate publisher xact_rollback for decoded transactions'
+);
+
+done_testing();
--
2.50.1 (Apple Git-155)
* Re: xact_rollback spikes when logical walsender exits
@ 2026-07-08 17:37 Nikolay Samokhvalov <nik@postgres.ai>
parent: Fujii Masao <masao.fujii@gmail.com>
0 siblings, 2 replies; 8+ messages in thread
From: Nikolay Samokhvalov @ 2026-07-08 17:37 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
Fujii-san, coming back to your questions in this thread, I should have
addressed
them before sending v2 -- apologies for jumping past them.
On Mon, Apr 20, 2026 at 9:35 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> Thanks for the report and patch!
>
> How to implement a solution depends on what xact_rollback in
> pg_stat_database
> is intended to mean. So at first we should consider which rollbacks should
> it count? The documentation does not currently give an explicit definition.
>
> At present, xact_rollback appears to count all rollbacks, explicit or
> implicit,
> by any process connected to the database, including regular backends,
> autovacuum workers, and logical walsenders. If that is the intended
> definition,
> then rollbacks implicitly performed by logical walsenders during logical
> replication should also be counted. Of course, even if we keep that
> definition,
> the sudden increase in xact_rollback might still be a problem, so we might
> need to call pgstat_report_stat() immediately after pgstat_flush_io() in
> walsender, so the counters continue to be updated periodically during
> logical replication.
>
> On the other hand, your patch seems to assume a different definition: that
> xact_rollback should count all explicit and implicit rollbacks, except
> those
> performed by logical walsenders during logical replication. That would be
> one possible approach, although it seems a bit odd to exclude only one
> subset
> of rollbacks.
>
> A third option would be to define xact_rollback more narrowly, counting
> only
> rollbacks by regular backends, and excluding rollbacks by processes such as
> autovacuum or walsender. At least in my view, xact_commit and xact_rollback
> in pg_stat_database are typically used by DBAs to check whether
> client transactions are committing or rolling back as expected. From
> that perspective, it seems intuitive for xact_rollback to count only
> rollbacks
> by regular backends. But others may reasonably see it differently.
>
On the definitional question: I'd argue the walsender case isn't a
"subset of rollbacks being excluded". The transactions being decoded
actually committed; the abort in ReorderBufferProcessTXN() is
internal cleanup of the catalog-snapshot transaction, not a
transaction outcome. Notably, xact_commit is not incremented for
these, so on any publisher the commit/rollback pair becomes
internally inconsistent in proportion to decoded throughput. So
counting these as rollbacks seems like a miscount under any of the
three definitions.
On option 1 (periodic pgstat_report_stat() in walsender): that would
trade the exit spike for a steady inflated rollback rate, which
seems worse for the metric's usefulness. Periodic flushing may still
be worth doing independently for other stats; happy to send that as
a separate patch.
On option 3: I agree this matches how DBAs actually use these
counters, and I'd support it. Since it changes observable behavior
for autovacuum etc., it seems like master-only material, together
with explicitly documenting the definition.
Concretely, I propose splitting:
1) 0001 (v3, attached): narrow fix for the walsender miscount,
intended as a back-patchable bug fix -- this is what's paging people
in production today. Rebased on current master; the TAP test now
also verifies that xact_commit does not change as a function of
decoded transactions (walsender shutdown has a small fixed
bookkeeping delta, handled via a control run).
2) 0002 (attached, draft): master-only implementation of option 3 --
count xact_commit/xact_rollback only for regular client backends --
plus a doc change defining both columns explicitly. It also removes
the now-redundant parallel argument from AtEOXact_PgStat() and
AtEOXact_PgStat_Database(), since parallel workers are background
workers and are excluded by the backend-type check. 0002 has no test
yet; if the definition is agreed on, I'll extend the new TAP test to
cover the autovacuum/walsender exclusion.
One consequence of option 3 worth stating explicitly: logical
replication apply workers are background workers, so transactions
they replay on a subscriber would no longer be counted in the
subscriber's xact_commit. That's arguably correct under the "client
transactions" definition, but it does change what subscriber-side
monitoring sees, so I wanted to flag it rather than have it
discovered later.
Added to the July commitfest: https://commitfest.postgresql.org/patch/6992/
Looking forward to your thoughts.
Nik
Attachments: download all (2 files, .tar.gz)
v3-0002-count-client-xacts-only.patch (application/octet-stream, 8.0K) download
From df6f10bd9ddf2b5659bf19d928763530a9af685c Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <nik@postgres.ai>
Date: Wed, 8 Jul 2026 08:04:58 -0700
Subject: [PATCH v3 2/2] Count xact_commit/xact_rollback only for client
backends
pg_stat_database.xact_commit and xact_rollback have historically counted
every top-level transaction end in a database, including the internal
transactions run by other server processes such as autovacuum workers and
walsenders. Those transactions are not user-visible transaction outcomes,
and counting them makes the ratio of commits to rollbacks harder to
interpret for monitoring.
Adopt the narrower definition discussed on the list: count a transaction in
xact_commit/xact_rollback only when it is executed by a regular client
backend (AmRegularBackendProcess()). The "parallel" argument, previously
used to keep parallel workers out of these counters, is now redundant --
parallel workers are background workers and are excluded by the backend-type
test -- so it is removed from both AtEOXact_PgStat() and
AtEOXact_PgStat_Database().
This is a monitoring-visible behavior change beyond the server processes
themselves: logical replication apply workers are background workers, so the
transactions they replay on a subscriber are no longer counted in the
subscriber's xact_commit. That is intended, and is called out here so the
change in subscriber statistics is not mistaken for a regression.
Because it changes long-standing counter semantics, this is master-only.
The back-patchable fix for the specific logical-decoding case that prompted
this is handled separately; this commit implements the general definition on
top of it.
Discussion: https://postgr.es/m/CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb%3DQ2KzTw%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 12 ++++++++----
src/backend/access/transam/twophase.c | 2 +-
src/backend/access/transam/xact.c | 4 ++--
src/backend/utils/activity/pgstat_database.c | 14 +++++++++++---
src/backend/utils/activity/pgstat_xact.c | 4 ++--
src/include/pgstat.h | 2 +-
src/include/utils/pgstat_internal.h | 2 +-
7 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 12b9ee20d4a..7e0a7550fa6 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3895,8 +3895,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>xact_commit</structfield> <type>bigint</type>
</para>
<para>
- Number of transactions in this database that have been
- committed
+ Number of transactions in this database that have been committed by
+ regular client backends. Transactions performed by other server
+ processes, such as autovacuum workers, walsenders, and logical
+ replication apply workers, are not counted.
</para></entry>
</row>
@@ -3905,8 +3907,10 @@ description | Waiting for a newly initialized WAL file to reach durable storage
<structfield>xact_rollback</structfield> <type>bigint</type>
</para>
<para>
- Number of transactions in this database that have been
- rolled back
+ Number of transactions in this database that have been rolled back by
+ regular client backends. Transactions performed by other server
+ processes, such as autovacuum workers, walsenders, and logical
+ replication apply workers, are not counted.
</para></entry>
</row>
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 1035e8b3fc7..0408f8794b4 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1676,7 +1676,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
LWLockRelease(TwoPhaseStateLock);
/* Count the prepared xact as committed or aborted */
- AtEOXact_PgStat(isCommit, false);
+ AtEOXact_PgStat(isCommit);
/*
* And now we can clean up any files we may have left.
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index d74b231caf3..4e627e3306b 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2515,7 +2515,7 @@ CommitTransaction(void)
AtEOXact_ComboCid();
AtEOXact_HashTables(true);
AtEOXact_RI(true);
- AtEOXact_PgStat(true, is_parallel_worker);
+ AtEOXact_PgStat(true);
AtEOXact_Snapshot(true, false);
AtEOXact_ApplyLauncher(true);
AtEOXact_LogicalRepWorkers(true);
@@ -3042,7 +3042,7 @@ AbortTransaction(void)
AtEOXact_ComboCid();
AtEOXact_HashTables(false);
AtEOXact_RI(false);
- AtEOXact_PgStat(false, is_parallel_worker);
+ AtEOXact_PgStat(false);
AtEOXact_ApplyLauncher(false);
AtEOXact_LogicalRepWorkers(false);
AtEOXact_LogicalCtl();
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 7f3bc016593..efe546931e3 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -17,6 +17,7 @@
#include "postgres.h"
+#include "miscadmin.h"
#include "storage/standby.h"
#include "utils/pgstat_internal.h"
#include "utils/timestamp.h"
@@ -292,10 +293,17 @@ pgstat_fetch_stat_dbentry(Oid dboid)
}
void
-AtEOXact_PgStat_Database(bool isCommit, bool parallel)
+AtEOXact_PgStat_Database(bool isCommit)
{
- /* Don't count parallel worker transaction stats */
- if (!parallel)
+ /*
+ * Only count transactions of regular client backends in
+ * xact_commit/xact_rollback. Transactions performed by other server
+ * processes -- autovacuum workers, walsenders, and, less obviously, logical
+ * replication apply workers -- are not user-visible transaction outcomes and
+ * would otherwise inflate these counters. (This subsumes the previous
+ * !parallel check, since parallel workers are bgworkers.)
+ */
+ if (AmRegularBackendProcess())
{
/*
* Count transaction commit or abort. (We use counters, not just
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c
index d1be3733e4d..6c3944b5f22 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -38,7 +38,7 @@ static bool pgStatSkipXactCounters = false;
* Called from access/transam/xact.c at top-level transaction commit/abort.
*/
void
-AtEOXact_PgStat(bool isCommit, bool parallel)
+AtEOXact_PgStat(bool isCommit)
{
PgStat_SubXactStatus *xact_state;
bool skip_xact_counters = pgStatSkipXactCounters;
@@ -49,7 +49,7 @@ AtEOXact_PgStat(bool isCommit, bool parallel)
*/
pgStatSkipXactCounters = false;
if (!skip_xact_counters)
- AtEOXact_PgStat_Database(isCommit, parallel);
+ AtEOXact_PgStat_Database(isCommit);
/* handle transactional stats information */
xact_state = pgStatXactStack;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index ff0a2e16467..4fa84061057 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -827,7 +827,7 @@ extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid);
* Functions in pgstat_xact.c
*/
-extern void AtEOXact_PgStat(bool isCommit, bool parallel);
+extern void AtEOXact_PgStat(bool isCommit);
extern void pgstat_suppress_xact_counters(void);
extern void pgstat_clear_xact_counter_suppression(void);
extern void AtEOSubXact_PgStat(bool isCommit, int nestDepth);
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index b3dc3ff7d8b..ad9bb649e34 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -737,7 +737,7 @@ extern void pgstat_checkpointer_snapshot_cb(void);
extern void pgstat_report_disconnect(Oid dboid);
extern void pgstat_update_dbstats(TimestampTz ts);
-extern void AtEOXact_PgStat_Database(bool isCommit, bool parallel);
+extern void AtEOXact_PgStat_Database(bool isCommit);
extern PgStat_StatDBEntry *pgstat_prep_database_pending(Oid dboid);
extern void pgstat_reset_database_timestamp(Oid dboid, TimestampTz ts);
--
2.50.1 (Apple Git-155)
v3-0001-xact_rollback-decoding-fix.patch (application/octet-stream, 13.1K) download
From 69ffad6af7a8bbbabaf3030ca417463a96cfb0c8 Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <nik@postgres.ai>
Date: Wed, 8 Jul 2026 06:11:24 +0000
Subject: [PATCH v3 1/2] Do not attribute logical decoding aborts to
xact_rollback
Logical decoding aborts the current transaction after decoding committed
transactions to clean up catalog access and other transaction-local state. In a
logical walsender this can be a top-level abort, so
pg_stat_database.xact_rollback is incremented even though no user-visible
transaction rolled back.
Keep these internal cleanup aborts out of xact_rollback by adding
AbortCurrentTransactionWithoutXactCounters(), a narrow wrapper around
AbortCurrentTransaction() that suppresses only the next pg_stat_database
xact_commit/xact_rollback counter update while preserving the rest of
transaction cleanup.
Add a TAP test that fails without the fix: five committed transactions decoded
by a subscription produce a publisher xact_rollback delta of 5 when the
walsender exits. With the fix, the xact_rollback delta remains 0. The test
also verifies that decoded transactions do not affect xact_commit by comparing
against a control walsender shutdown with no decoded transactions.
Reported-by: Rafael Thofehrn Castro
Discussion: https://postgr.es/m/CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb%3DQ2KzTw%40mail.gmail.com
---
src/backend/access/transam/xact.c | 23 ++++
.../replication/logical/reorderbuffer.c | 18 ++-
src/backend/utils/activity/pgstat_xact.c | 32 ++++-
src/include/access/xact.h | 1 +
src/include/pgstat.h | 2 +
src/test/subscription/meson.build | 1 +
.../t/039_publisher_xact_rollback.pl | 124 ++++++++++++++++++
7 files changed, 196 insertions(+), 5 deletions(-)
create mode 100644 src/test/subscription/t/039_publisher_xact_rollback.pl
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 3a89149016f..d74b231caf3 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -3512,6 +3512,29 @@ AbortCurrentTransaction(void)
}
}
+/*
+ * AbortCurrentTransactionWithoutXactCounters
+ *
+ * Like AbortCurrentTransaction(), but don't count the abort in
+ * pg_stat_database.xact_rollback. For internal cleanup aborts that don't
+ * represent a user-visible transaction outcome.
+ */
+void
+AbortCurrentTransactionWithoutXactCounters(void)
+{
+ pgstat_suppress_xact_counters();
+ AbortCurrentTransaction();
+
+ /*
+ * AbortCurrentTransaction() is a no-op when already idle (e.g. an abort at
+ * TBLOCK_DEFAULT/TRANS_DEFAULT), in which case AtEOXact_PgStat() never runs
+ * to consume the flag. That can't happen at the current call sites, which
+ * always abort a live transaction, but clear it unconditionally anyway so
+ * set and clear stay paired defensively and the flag cannot leak.
+ */
+ pgstat_clear_xact_counter_suppression();
+}
+
/*
* AbortCurrentTransactionInternal - a function doing an iteration of work
* regarding handling the current transaction abort. In the case of
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 059ed860314..49bb87ab1ec 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2666,9 +2666,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
* Aborting the current (sub-)transaction as a whole has the right
* semantics. We want all locks acquired in here to be released, not
* reassigned to the parent and we do not want any database access
- * have persistent effects.
+ * have persistent effects. In the !using_subtxn case this is a
+ * top-level abort for internal cleanup; keep it out of
+ * pg_stat_database.xact_rollback.
*/
- AbortCurrentTransaction();
+ if (using_subtxn)
+ AbortCurrentTransaction();
+ else
+ AbortCurrentTransactionWithoutXactCounters();
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
@@ -2729,9 +2734,14 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/*
* Force cache invalidation to happen outside of a valid transaction
- * to prevent catalog access as we just caught an error.
+ * to prevent catalog access as we just caught an error. As above,
+ * keep the top-level internal cleanup abort out of
+ * pg_stat_database.xact_rollback.
*/
- AbortCurrentTransaction();
+ if (using_subtxn)
+ AbortCurrentTransaction();
+ else
+ AbortCurrentTransactionWithoutXactCounters();
/* make sure there's no cache pollution */
if (rbtxn_distr_inval_overflowed(txn))
diff --git a/src/backend/utils/activity/pgstat_xact.c b/src/backend/utils/activity/pgstat_xact.c
index 3e1978775e1..d1be3733e4d 100644
--- a/src/backend/utils/activity/pgstat_xact.c
+++ b/src/backend/utils/activity/pgstat_xact.c
@@ -31,6 +31,7 @@ static void AtEOSubXact_PgStat_DroppedStats(PgStat_SubXactStatus *xact_state,
bool isCommit, int nestDepth);
static PgStat_SubXactStatus *pgStatXactStack = NULL;
+static bool pgStatSkipXactCounters = false;
/*
@@ -40,8 +41,15 @@ void
AtEOXact_PgStat(bool isCommit, bool parallel)
{
PgStat_SubXactStatus *xact_state;
+ bool skip_xact_counters = pgStatSkipXactCounters;
- AtEOXact_PgStat_Database(isCommit, parallel);
+ /*
+ * Consume the suppression flag. Only the xact_commit/xact_rollback bump is
+ * skipped; transactional stats below must still be processed.
+ */
+ pgStatSkipXactCounters = false;
+ if (!skip_xact_counters)
+ AtEOXact_PgStat_Database(isCommit, parallel);
/* handle transactional stats information */
xact_state = pgStatXactStack;
@@ -59,6 +67,28 @@ AtEOXact_PgStat(bool isCommit, bool parallel)
pgstat_clear_snapshot();
}
+/*
+ * Suppress the xact_commit/xact_rollback bump at the next top-level transaction
+ * end. For internal aborts that don't represent a user-visible transaction
+ * outcome; see AbortCurrentTransactionWithoutXactCounters().
+ */
+void
+pgstat_suppress_xact_counters(void)
+{
+ Assert(!pgStatSkipXactCounters);
+ pgStatSkipXactCounters = true;
+}
+
+/*
+ * Clear the suppression flag set by pgstat_suppress_xact_counters(), in case
+ * the intervening abort never reached AtEOXact_PgStat() to consume it.
+ */
+void
+pgstat_clear_xact_counter_suppression(void)
+{
+ pgStatSkipXactCounters = false;
+}
+
/*
* When committing, drop stats for objects dropped in the transaction. When
* aborting, drop stats for objects created in the transaction.
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index a8cbdf247c8..9a22a2e9a67 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -467,6 +467,7 @@ extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s);
extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s);
extern void CommitTransactionCommand(void);
extern void AbortCurrentTransaction(void);
+extern void AbortCurrentTransactionWithoutXactCounters(void);
extern void BeginTransactionBlock(void);
extern bool EndTransactionBlock(bool chain);
extern bool PrepareTransactionBlock(const char *gid);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 58a44857f13..ff0a2e16467 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -828,6 +828,8 @@ extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid);
*/
extern void AtEOXact_PgStat(bool isCommit, bool parallel);
+extern void pgstat_suppress_xact_counters(void);
+extern void pgstat_clear_xact_counter_suppression(void);
extern void AtEOSubXact_PgStat(bool isCommit, int nestDepth);
extern void AtPrepare_PgStat(void);
extern void PostPrepare_PgStat(void);
diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build
index e71e95c6297..268fa8c3e9c 100644
--- a/src/test/subscription/meson.build
+++ b/src/test/subscription/meson.build
@@ -48,6 +48,7 @@ tests += {
't/036_sequences.pl',
't/037_except.pl',
't/038_walsnd_shutdown_timeout.pl',
+ 't/039_publisher_xact_rollback.pl',
't/100_bugs.pl',
],
},
diff --git a/src/test/subscription/t/039_publisher_xact_rollback.pl b/src/test/subscription/t/039_publisher_xact_rollback.pl
new file mode 100644
index 00000000000..c75634425d0
--- /dev/null
+++ b/src/test/subscription/t/039_publisher_xact_rollback.pl
@@ -0,0 +1,124 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+
+# Check that pg_stat_database.xact_rollback on a logical-replication
+# publisher is not inflated by the walsender's internal catalog-cleanup
+# aborts. ReorderBufferProcessTXN() ends each decoded transaction with
+# AbortCurrentTransaction(); in the walsender that is a top-level abort.
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+sub get_xact_stats
+{
+ my ($node) = @_;
+
+ return split /\|/, $node->safe_psql('template1', q{
+ SELECT xact_commit, xact_rollback
+ FROM pg_stat_database
+ WHERE datname = 'postgres'
+ });
+}
+
+# Wait for this subscription's walsender to disappear from pg_stat_activity.
+#
+# A walsender registers its stats flush (pgstat_shutdown_hook) with
+# before_shmem_exit() and clears its pg_stat_activity entry
+# (pgstat_beshutdown_hook) with on_shmem_exit(). shmem_exit() runs all
+# before_shmem_exit callbacks before any on_shmem_exit callback, so by the time
+# the walsender is gone from pg_stat_activity its xact_commit/xact_rollback
+# counts have already been flushed to shared stats. Observing its exit is
+# therefore sufficient synchronization for reading the flushed counters.
+sub wait_for_walsender_exit
+{
+ my ($node) = @_;
+
+ $node->poll_query_until(
+ 'template1', q{
+ SELECT count(*) = 0 FROM pg_stat_activity
+ WHERE backend_type = 'walsender' AND application_name = 's'
+ })
+ or die 's walsender did not exit';
+}
+
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+# Autovacuum would commit on the postgres database and perturb the xact_commit
+# delta this test compares between the control and decoding runs.
+$node_publisher->append_conf('postgresql.conf', 'autovacuum = off');
+$node_publisher->start;
+
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->start;
+
+$node_publisher->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+$node_subscriber->safe_psql('postgres',
+ 'CREATE TABLE t (id int PRIMARY KEY)');
+
+$node_publisher->safe_psql('postgres', 'CREATE PUBLICATION p FOR TABLE t');
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION s CONNECTION '$publisher_connstr' PUBLICATION p");
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 's');
+
+# Measure a walsender shutdown without any decoded committed transactions.
+# The absolute xact_commit delta is not asserted: walsender shutdown performs
+# fixed session/transaction bookkeeping that may change independently of this
+# test. The control run captures that non-decoding delta so the decoding run
+# can assert it is unchanged by decoded transactions.
+my ($control_base_commit, $control_base_rollback) =
+ get_xact_stats($node_publisher);
+
+$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE');
+wait_for_walsender_exit($node_publisher);
+
+my ($control_final_commit, $control_final_rollback) =
+ get_xact_stats($node_publisher);
+
+my $control_commit_delta = $control_final_commit - $control_base_commit;
+my $control_rollback_delta = $control_final_rollback - $control_base_rollback;
+
+$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s ENABLE');
+
+# Five autocommit INSERTs: each becomes one decoded committed transaction on
+# the walsender. Without the fix, those produce five spurious rollbacks after
+# DISABLE. xact_commit may change due to fixed walsender bookkeeping, but it
+# must not change as a function of decoded transactions.
+my $n = 5;
+$node_publisher->safe_psql('postgres',
+ join('', map { "INSERT INTO t VALUES ($_);\n" } 1 .. $n));
+
+$node_publisher->wait_for_catchup('s');
+
+# Baseline after catchup and before walsender exit, so the deltas measure only
+# the stats flushed by this subscription's walsender during shutdown.
+my ($base_commit, $base_rollback) = get_xact_stats($node_publisher);
+
+# Disabling the subscription terminates the walsender; its shutdown hook
+# flushes pgstat counters to shared stats before it leaves pg_stat_activity.
+$node_subscriber->safe_psql('postgres', 'ALTER SUBSCRIPTION s DISABLE');
+wait_for_walsender_exit($node_publisher);
+
+my ($final_commit, $final_rollback) = get_xact_stats($node_publisher);
+
+cmp_ok(
+ $control_rollback_delta, '==', 0,
+ 'walsender shutdown without decoded transactions does not inflate publisher xact_rollback'
+);
+
+cmp_ok(
+ $final_rollback - $base_rollback, '==', 0,
+ 'walsender does not inflate publisher xact_rollback for decoded transactions'
+);
+
+cmp_ok(
+ $final_commit - $base_commit, '==', $control_commit_delta,
+ 'walsender does not change publisher xact_commit as a function of decoded transactions'
+);
+
+done_testing();
--
2.50.1 (Apple Git-155)
* Re: xact_rollback spikes when logical walsender exits
@ 2026-07-09 04:56 jihyun bahn <rring0727@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 0 replies; 8+ messages in thread
From: jihyun bahn @ 2026-07-09 04:56 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Nik Samokhvalov <nik@postgres.ai>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed
Hi Nikolay,
I tested the v3 patches on master (commit e01b23b) on Ubuntu 26.04,
gcc 15.2.0, configured with --enable-cassert --enable-debug
--enable-tap-tests.
0001:
Applies cleanly and builds without new warnings. To verify that the new
TAP test actually catches the bug, I first added only
t/039_publisher_xact_rollback.pl on top of unpatched master: it fails as
intended (the xact_rollback delta was 5, expected 0). With the full
patch applied it passes 3/3. make check (245 tests) and the full
subscription suite (40 files / 588 tests) pass with 0001 applied.
I also reproduced the problem at a larger scale: 1000 autocommit INSERTs
decoded by a subscription, then ALTER SUBSCRIPTION ... DISABLE.
Publisher pg_stat_database (xact_commit, xact_rollback), read via
template1 connections, autovacuum off:
master 0001
before DISABLE 1013, 0 1013, 0
after DISABLE 1015, 1000 1015, 0
On unpatched master the walsender exit adds exactly one spurious
rollback per decoded transaction; with 0001 the delta is 0 and every
other counter is identical. Operationally this is exactly the
false-positive alert pattern: the spike lands on routine actions
(subscription disable, slot drop, restarts) while actual rollbacks are
zero. Each decoded transaction was already counted once in xact_commit
by the committing client backend, and the rollback that appears later
belongs to the walsender's own internal catalog-access transaction --
so monitoring ends up seeing one commit plus one rollback for work
that in fact committed cleanly. Since both option 2 and option 3
discussed upthread imply excluding these internal aborts, 0001 seems
reasonable to proceed with as a narrow back-patchable fix while the
wider definitional question is settled around 0002.
0002 (draft):
Builds on top of 0001 and behaves as described; make installcheck-world
also passes with both patches applied. I measured the
subscriber-side visibility change the commit message calls out:
between a baseline taken after initial table sync and a reading taken
after catchup (before disabling the subscription), subscriber
xact_commit moves by +1001 on master/0001 -- the 1000 replayed
transactions plus one further commit from a non-client backend -- and
by 0 with 0002. The subsequent ALTER SUBSCRIPTION ... DISABLE session
adds +2 to xact_commit in all cases, as expected for a regular client
backend.
One more data point in favor of the narrower definition: on
master/0001, disabling the subscription also adds +1 to subscriber
xact_rollback. That appears to be maybe_reread_subscription(), which
starts a transaction to reread pg_subscription, notices the
subscription was disabled, and proc_exit()s from within that
transaction, so the shutdown abort of an internal catalog-read
transaction is counted -- the same pattern as the walsender case, on
the subscriber side. 0002 removes that as well.
As a DBA who watches these counters across a fleet: +1 to option 3.
xact_commit/xact_rollback are read as "are application transactions
healthy", and background-process noise makes the commit/rollback ratio
harder to trust.
One question on observability after 0002: subscriber xact_commit is
today the only cheap proxy for logical-replication apply throughput in
transactions. pg_stat_subscription_stats currently exposes error and
conflict counts but no applied-commit count, and pg_stat_subscription
is LSN-based. Would it make sense to point at an alternative in the
docs, or could an apply-commit counter in pg_stat_subscription_stats be
worth considering as a follow-up?
If it helps, I would be happy to draft a TAP test for 0002 along the
lines of 0001's test (client backend counted; autovacuum, walsender,
and apply worker excluded).
Tested-by: Jihyun Bahn <rring0727@gmail.com>
Best regards,
Jihyun Bahn
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
@ 2026-09-08 07:54 Mircea Cadariu <cadariu.mircea@gmail.com>
parent: Nikolay Samokhvalov <nik@postgres.ai>
1 sibling, 1 reply; 8+ messages in thread
From: Mircea Cadariu @ 2026-09-08 07:54 UTC (permalink / raw)
To: Nikolay Samokhvalov <nik@postgres.ai>; Fujii Masao <masao.fujii@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
Hi,
On 08/07/2026 19:37, Nikolay Samokhvalov wrote:
> Concretely, I propose splitting:
>
> 1) 0001 (v3, attached): narrow fix for the walsender miscount,
> intended as a back-patchable bug fix -- this is what's paging people
> in production today. Rebased on current master; the TAP test now
> also verifies that xact_commit does not change as a function of
> decoded transactions (walsender shutdown has a small fixed
> bookkeeping delta, handled via a control run).
>
> 2) 0002 (attached, draft): master-only implementation of option 3 --
> count xact_commit/xact_rollback only for regular client backends --
> plus a doc change defining both columns explicitly. It also removes
> the now-redundant parallel argument from AtEOXact_PgStat() and
> AtEOXact_PgStat_Database(), since parallel workers are background
> workers and are excluded by the backend-type check. 0002 has no test
> yet; if the definition is agreed on, I'll extend the new TAP test to
> cover the autovacuum/walsender exclusion.
Thanks for submitting the v3 patch, I had a look.
The patch applies cleanly on master. As experiment I added the tests
only from 0001 and as expected they failed, but with the patch applied
they pass. So far so good.
While the approach in 0001 could be considered a bit "action at a
distance" approach, a suitable structural solution is in 0002.
I agree with starting to count xact_commit/xact_rollback only for
regular client backends. TBH I expected it works this way already.
Indeed, 0002 needs tests.
I'll set it to Ready for Committer and am volunteering for adding the
tests or any other aspect I can help with to progress this patch.
--
Thanks,
Mircea Cadariu
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
@ 2026-09-23 16:43 Nikolay Samokhvalov <nik@postgres.ai>
parent: Mircea Cadariu <cadariu.mircea@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Samokhvalov @ 2026-09-23 16:43 UTC (permalink / raw)
To: Mircea Cadariu <cadariu.mircea@gmail.com>; +Cc: Fujii Masao <masao.fujii@gmail.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; Rafael Thofehrn Castro <rafaelthca@gmail.com>
On Tue, Sep 8, 2026, Mircea Cadariu <cadariu.mircea@gmail.com> wrote:
> The patch applies cleanly on master. As experiment I added the tests
> only from 0001 and as expected they failed, but with the patch applied
> they pass. So far so good.
Thanks Mircea. A Windows CI failure while testing this series was in
isolationtester. It exited on connection loss from an intentionally
terminated test session before completing the permutation and teardown.
Andrey's discussion of the test-driver issue is here:
https://www.postgresql.org/message-id/088AB35D-0860-494A-A9CF-8B301543AA51%40yandex-team.ru
This is separate from the xact_rollback accounting change. 0002 still
needs tests.
Nik
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-09-23 16:43 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-17 15:15 xact_rollback spikes when logical walsender exits Nikolay Samokhvalov <nik@postgres.ai>
2026-04-20 16:35 ` Fujii Masao <masao.fujii@gmail.com>
2026-07-08 17:37 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-07-09 04:56 ` jihyun bahn <rring0727@gmail.com>
2026-09-08 07:54 ` Mircea Cadariu <cadariu.mircea@gmail.com>
2026-09-23 16:43 ` Nikolay Samokhvalov <nik@postgres.ai>
2026-04-21 06:38 ` vignesh C <vignesh21@gmail.com>
2026-05-13 00:06 ` Nikolay Samokhvalov <nik@postgres.ai>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox