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]> 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-09 04:56 jihyun bahn <[email protected]> parent: Nikolay Samokhvalov <[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