public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikolay Samokhvalov <[email protected]>
To: Fujii Masao <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Rafael Thofehrn Castro <[email protected]>
Subject: Re: xact_rollback spikes when logical walsender exits
Date: Wed, 8 Jul 2026 10:37:11 -0700
Message-ID: <CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com> (raw)
In-Reply-To: <CAHGQGwFDv6=Dcbf1YbGH5S7y-M4ar4-zC-c1GWco_Cn_SE0c7w@mail.gmail.com>
References: <CAM527d_EbU5Li4a5FdKQjYsdF-4Lqr_i3jXmZOm7Wbb=Q2KzTw@mail.gmail.com>
<CAHGQGwFDv6=Dcbf1YbGH5S7y-M4ar4-zC-c1GWco_Cn_SE0c7w@mail.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 <[email protected]> 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:
[application/octet-stream] v3-0002-count-client-xacts-only.patch (8.0K, ../CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com/3-v3-0002-count-client-xacts-only.patch)
download | inline diff:
From df6f10bd9ddf2b5659bf19d928763530a9af685c Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <[email protected]>
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)
[application/octet-stream] v3-0001-xact_rollback-decoding-fix.patch (13.1K, ../CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com/4-v3-0001-xact_rollback-decoding-fix.patch)
download | inline diff:
From 69ffad6af7a8bbbabaf3030ca417463a96cfb0c8 Mon Sep 17 00:00:00 2001
From: Nikolay Samokhvalov <[email protected]>
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)
view thread (3+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: xact_rollback spikes when logical walsender exits
In-Reply-To: <CAM527d8Evz3BCffJeL=1RJi-7jjPYbKz_ZoUxm2Go2i-_N+g6w@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox