public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v46 2/7] Add conditional lock feature to dshash
3+ messages / 3 participants
[nested] [flat]
* [PATCH v46 2/7] Add conditional lock feature to dshash
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw)
Dshash currently waits for lock unconditionally. It is inconvenient
when we want to avoid being blocked by other processes. This commit
adds alternative functions of dshash_find and dshash_find_or_insert
that allows immediate return on lock failure.
---
src/backend/lib/dshash.c | 98 +++++++++++++++++++++-------------------
src/include/lib/dshash.h | 3 ++
2 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index 520bfa0979..853d78b528 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -383,6 +383,10 @@ dshash_get_hash_table_handle(dshash_table *hash_table)
* the caller must take care to ensure that the entry is not left corrupted.
* The lock mode is either shared or exclusive depending on 'exclusive'.
*
+ * If found is not NULL, *found is set to true if the key is found in the hash
+ * table. If the key is not found, *found is set to false and a pointer to a
+ * newly created entry is returned.
+ *
* The caller must not lock a lock already.
*
* Note that the lock held is in fact an LWLock, so interrupts will be held on
@@ -392,36 +396,7 @@ dshash_get_hash_table_handle(dshash_table *hash_table)
void *
dshash_find(dshash_table *hash_table, const void *key, bool exclusive)
{
- dshash_hash hash;
- size_t partition;
- dshash_table_item *item;
-
- hash = hash_key(hash_table, key);
- partition = PARTITION_FOR_HASH(hash);
-
- Assert(hash_table->control->magic == DSHASH_MAGIC);
- Assert(!hash_table->find_locked);
-
- LWLockAcquire(PARTITION_LOCK(hash_table, partition),
- exclusive ? LW_EXCLUSIVE : LW_SHARED);
- ensure_valid_bucket_pointers(hash_table);
-
- /* Search the active bucket. */
- item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash));
-
- if (!item)
- {
- /* Not found. */
- LWLockRelease(PARTITION_LOCK(hash_table, partition));
- return NULL;
- }
- else
- {
- /* The caller will free the lock by calling dshash_release_lock. */
- hash_table->find_locked = true;
- hash_table->find_exclusively_locked = exclusive;
- return ENTRY_FROM_ITEM(item);
- }
+ return dshash_find_extended(hash_table, key, exclusive, false, false, NULL);
}
/*
@@ -439,31 +414,60 @@ dshash_find_or_insert(dshash_table *hash_table,
const void *key,
bool *found)
{
- dshash_hash hash;
- size_t partition_index;
- dshash_partition *partition;
+ return dshash_find_extended(hash_table, key, true, false, true, found);
+}
+
+
+/*
+ * Find the key in the hash table.
+ *
+ * "exclusive" is the lock mode in which the partition for the returned item
+ * is locked. If "nowait" is true, the function immediately returns if
+ * required lock was not acquired. "insert" indicates insert mode. In this
+ * mode new entry is inserted and set *found to false. *found is set to true if
+ * found. "found" must be non-null in this mode.
+ */
+void *
+dshash_find_extended(dshash_table *hash_table, const void *key,
+ bool exclusive, bool nowait, bool insert, bool *found)
+{
+ dshash_hash hash = hash_key(hash_table, key);
+ size_t partidx = PARTITION_FOR_HASH(hash);
+ dshash_partition *partition = &hash_table->control->partitions[partidx];
+ LWLockMode lockmode = exclusive ? LW_EXCLUSIVE : LW_SHARED;
dshash_table_item *item;
- hash = hash_key(hash_table, key);
- partition_index = PARTITION_FOR_HASH(hash);
- partition = &hash_table->control->partitions[partition_index];
-
- Assert(hash_table->control->magic == DSHASH_MAGIC);
- Assert(!hash_table->find_locked);
+ /* must be exclusive when insert allowed */
+ Assert(!insert || (exclusive && found != NULL));
restart:
- LWLockAcquire(PARTITION_LOCK(hash_table, partition_index),
- LW_EXCLUSIVE);
+ if (!nowait)
+ LWLockAcquire(PARTITION_LOCK(hash_table, partidx), lockmode);
+ else if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partidx),
+ lockmode))
+ return NULL;
+
ensure_valid_bucket_pointers(hash_table);
/* Search the active bucket. */
item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash));
if (item)
- *found = true;
+ {
+ if (found)
+ *found = true;
+ }
else
{
- *found = false;
+ if (found)
+ *found = false;
+
+ if (!insert)
+ {
+ /* The caller didn't told to add a new entry. */
+ LWLockRelease(PARTITION_LOCK(hash_table, partidx));
+ return NULL;
+ }
/* Check if we are getting too full. */
if (partition->count > MAX_COUNT_PER_PARTITION(hash_table))
@@ -479,7 +483,8 @@ restart:
* Give up our existing lock first, because resizing needs to
* reacquire all the locks in the right order to avoid deadlocks.
*/
- LWLockRelease(PARTITION_LOCK(hash_table, partition_index));
+ LWLockRelease(PARTITION_LOCK(hash_table, partidx));
+
resize(hash_table, hash_table->size_log2 + 1);
goto restart;
@@ -493,12 +498,13 @@ restart:
++partition->count;
}
- /* The caller must release the lock with dshash_release_lock. */
+ /* The caller will free the lock by calling dshash_release_lock. */
hash_table->find_locked = true;
- hash_table->find_exclusively_locked = true;
+ hash_table->find_exclusively_locked = exclusive;
return ENTRY_FROM_ITEM(item);
}
+
/*
* Remove an entry by key. Returns true if the key was found and the
* corresponding entry was removed.
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index a6ea377173..5b8114d041 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -91,6 +91,9 @@ extern void *dshash_find(dshash_table *hash_table,
const void *key, bool exclusive);
extern void *dshash_find_or_insert(dshash_table *hash_table,
const void *key, bool *found);
+extern void *dshash_find_extended(dshash_table *hash_table, const void *key,
+ bool exclusive, bool nowait, bool insert,
+ bool *found);
extern bool dshash_delete_key(dshash_table *hash_table, const void *key);
extern void dshash_delete_entry(dshash_table *hash_table, void *entry);
extern void dshash_release_lock(dshash_table *hash_table, void *entry);
--
2.27.0
----Next_Part(Thu_Jan_14_15_14_25_2021_903)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v46-0003-Make-archiver-process-an-auxiliary-process.patch"
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
@ 2026-07-08 17:37 Nikolay Samokhvalov <[email protected]>
2026-07-09 04:56 ` Re: xact_rollback spikes when logical walsender exits jihyun bahn <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Nikolay Samokhvalov @ 2026-07-08 17:37 UTC (permalink / raw)
To: Fujii Masao <[email protected]>; +Cc: pgsql-hackers <[email protected]>; Rafael Thofehrn Castro <[email protected]>
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)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: xact_rollback spikes when logical walsender exits
2026-07-08 17:37 Re: xact_rollback spikes when logical walsender exits Nikolay Samokhvalov <[email protected]>
@ 2026-07-09 04:56 ` jihyun bahn <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: jihyun bahn @ 2026-07-09 04:56 UTC (permalink / raw)
To: [email protected]; +Cc: Nik Samokhvalov <[email protected]>
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 <[email protected]>
Best regards,
Jihyun Bahn
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-07-09 04:56 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v46 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]>
2026-07-08 17:37 Re: xact_rollback spikes when logical walsender exits Nikolay Samokhvalov <[email protected]>
2026-07-09 04:56 ` Re: xact_rollback spikes when logical walsender exits jihyun bahn <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox