Re: xact_rollback spikes when logical walsender exits

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

Hi Nikolay,

I tested the v3 patches on master (commit e01b23b) on Ubuntu 26.04,
gcc 15.2.0, configured with --enable-cassert --enable-debug
--enable-tap-tests.

0001:

Applies cleanly and builds without new warnings. To verify that the new
TAP test actually catches the bug, I first added only
t/039_publisher_xact_rollback.pl on top of unpatched master: it fails as
intended (the xact_rollback delta was 5, expected 0). With the full
patch applied it passes 3/3. make check (245 tests) and the full
subscription suite (40 files / 588 tests) pass with 0001 applied.

I also reproduced the problem at a larger scale: 1000 autocommit INSERTs
decoded by a subscription, then ALTER SUBSCRIPTION ... DISABLE.
Publisher pg_stat_database (xact_commit, xact_rollback), read via
template1 connections, autovacuum off:

master 0001
before DISABLE 1013, 0 1013, 0
after DISABLE 1015, 1000 1015, 0

On unpatched master the walsender exit adds exactly one spurious
rollback per decoded transaction; with 0001 the delta is 0 and every
other counter is identical. Operationally this is exactly the
false-positive alert pattern: the spike lands on routine actions
(subscription disable, slot drop, restarts) while actual rollbacks are
zero. Each decoded transaction was already counted once in xact_commit
by the committing client backend, and the rollback that appears later
belongs to the walsender's own internal catalog-access transaction --
so monitoring ends up seeing one commit plus one rollback for work
that in fact committed cleanly. Since both option 2 and option 3
discussed upthread imply excluding these internal aborts, 0001 seems
reasonable to proceed with as a narrow back-patchable fix while the
wider definitional question is settled around 0002.

0002 (draft):

Builds on top of 0001 and behaves as described; make installcheck-world
also passes with both patches applied. I measured the
subscriber-side visibility change the commit message calls out:
between a baseline taken after initial table sync and a reading taken
after catchup (before disabling the subscription), subscriber
xact_commit moves by +1001 on master/0001 -- the 1000 replayed
transactions plus one further commit from a non-client backend -- and
by 0 with 0002. The subsequent ALTER SUBSCRIPTION ... DISABLE session
adds +2 to xact_commit in all cases, as expected for a regular client
backend.

One more data point in favor of the narrower definition: on
master/0001, disabling the subscription also adds +1 to subscriber
xact_rollback. That appears to be maybe_reread_subscription(), which
starts a transaction to reread pg_subscription, notices the
subscription was disabled, and proc_exit()s from within that
transaction, so the shutdown abort of an internal catalog-read
transaction is counted -- the same pattern as the walsender case, on
the subscriber side. 0002 removes that as well.

As a DBA who watches these counters across a fleet: +1 to option 3.
xact_commit/xact_rollback are read as "are application transactions
healthy", and background-process noise makes the commit/rollback ratio
harder to trust.

One question on observability after 0002: subscriber xact_commit is
today the only cheap proxy for logical-replication apply throughput in
transactions. pg_stat_subscription_stats currently exposes error and
conflict counts but no applied-commit count, and pg_stat_subscription
is LSN-based. Would it make sense to point at an alternative in the
docs, or could an apply-commit counter in pg_stat_subscription_stats be
worth considering as a follow-up?

If it helps, I would be happy to draft a TAP test for 0002 along the
lines of 0001's test (client backend counted; autovacuum, walsender,
and apply worker excluded).

Tested-by: Jihyun Bahn <rring0727(at)gmail(dot)com>

Best regards,
Jihyun Bahn