Re: xact_rollback spikes when logical walsender exits - Mailing list pgsql-hackers

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