Re: pg_xmin_horizon: a system view of everything pinning the xmin horizon

Thanks for the review.

On Tuesday, September 15th, 2026 at 1:28 PM, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> Good catch, and the new wording looks right. datfrozenxid can end
> up older than the oldest relfrozenxid in the database, because the
> computation starts from the oldest xid vacuum is allowed to remove,
> not from the tables. I recommend proposing this separately and
> probably back-patch it, but fine to keep it here too.
>
> aggressive VACUUM). Similarly, the
> <structfield>datfrozenxid</structfield> column of a database's
> <structname>pg_database</structname> row is a lower bound on the
> unfrozen XIDs

1. I'll keep it here.

> In both of the following places, can we have a sample query and
> output with all the per-class rows present? IMO, that works better and
> readily usable for those looking at it.

2. Examples would add little here.

> A few wording nits, mostly about matching what the docs already
> say. The steps that follow say "old prepared transactions",
> "long-running open transactions" and "old replication slots".
> system-views.sgml says "backend currently holding a snapshot or
> transaction ID". monitoring.sgml calls this the xmin horizon, and
> describes the fourth kind as a standby's xmin horizon reported by
> hot_standby_feedback, written without hyphens as the docs do
> everywhere. And for shared, the docs' usual phrase is shared catalogs.
>
> How about changing "snapshot-holding backend" to "backend holding a
> snapshot or transaction ID", "per-class xmin contribution" to the
> column names shared_xmin, catalog_xmin and data_xmin (or to xmin
> horizons), "hot-standby-feedback sender" to "hot standby feedback
> sender", and "cluster-wide shared horizon" to the horizon for shared
> catalogs? Same comments applicable elsewhere in the patch.

3. Done.

> The steps for finding the cause of the wraparound warnings
> currently point at pg_stat_activity, pg_prepared_xacts and
> pg_replication_slots, and the patch replaces those with the new view.
> The intent makes sense, since the whole point of pg_xmin_horizon is to
> be the first place to look. But the existing methods are not wrong
> either, and there are scripts and runbooks built around them. So how
> about mentioning both, something like "You can find these in
> pg_xmin_horizon or existing views"? There will be no surprises for
> those relying on the existing views, and they can move to the new one
> if they wish.

4. Existing methods are wrong. No existing view exposes the
statusFlags ComputeXidHorizons() reads, so pg_stat_activity shows a
backend_xmin for backends the horizons skip, and the wraparound steps
treat every large backend_xmin as a pin. As one consequence, a user
following the steps terminates a lazy VACUUM for its large
backend_xmin, though it pins nothing and freezes the old XIDs that
cause the warning.

> How about "sources holding back the cluster's xmin horizon,
> preventing VACUUM from removing dead row versions and clearing old
> XIDs"? Matches what monitoring.sgml and maintenance.sgml say. Same
> comment applies to the other places in the patch that say removal
> horizon.

5. Done.

> "physical-replication sender" is a new word here, and calling it a
> sender is confusing when what it holds is an xmin the standby sent to
> it. How about "a standby_feedback row represents a WAL sender process
> without a replication slot, holding the xmin horizon sent by its
> standby through hot_standby_feedback"?

6. Done.

> Why join with pg_prepared_xacts here? The point of pg_xmin_horizon
> was to collect most of the things in C to avoid joining existing
> views. The join also means gid reads null if the prepared transaction
> ends in between, for a row the view still reports. Why not return gid
> from the function itself, same as xact_start?

7. No, the point was not to avoid joining existing views. The main
points were to avoid transcribing the horizon policy logic from C to
SQL and to avoid publishing implementation details in public views.
v6 replaced the pg_stat_activity join to get fresh data, not because
joins are bad. Which locks would the function hold while it reads the
gid? How would emitting null gids for these rows adversely impact the
user?

> How about using "<literal>replication_slot</literal> and
> <literal>standby_feedback</literal> rows pin the horizon ..."?

8. Done.

> This seems unnecessary. The previous sentence already says these
> rows pin cluster-wide regardless of datid, so the note about filtering
> on datid adds little.

9. Done.

> "The transaction ID this row owns, or null if it owns none" reads
> a bit confusing. How about "Transaction identifier of this row's
> source, if any; see <xref linkend="transaction-id"/>" similar to
> pg_stat_activity.backend_xid?

10. Done.

> How about listing all the columns here, shared_xmin, catalog_xmin
> and data_xmin, for clarity?

11. Done.

> Same point as the per-class one above. How about "The transaction
> ID this row pins for the shared xmin horizon. Null if it does not pin
> that horizon", and likewise for catalog_xmin and data_xmin? The
> data_xmin entry then reads "pins none of the three horizons and shows
> null in shared_xmin, catalog_xmin and data_xmin".

12. Done.

> How about following the existing columns here, something like
> "Time when this row's transaction was started, or, for prepared_xact
> rows, the time at which the transaction was prepared for commit",
> similar to pg_stat_activity.xact_start and pg_prepared_xacts.prepared?

13. The replacement would say the same thing with more words.

> This paragraph took me a couple of passes to follow although I
> understand how these contribute to overall XID horizon in
> ComputeXidHorizons(). How about something shorter, in the order of
> what a reader needs?
>
> For replication_slot rows, shared_xmin and catalog_xmin report the
> older of the slot's xmin and catalog_xmin, while data_xmin reports the
> slot's xmin alone, because a slot's catalog_xmin applies to catalog
> rows only, not to dead row versions in user tables. These are the
> slot's effective xmins (the values the server uses when it computes
> the horizon), so while a slot is being created they can briefly differ
> from the ones shown in pg_replication_slots. The server also keeps a
> single oldest value across all slots, recomputed only when a slot
> changes, so for a short while it can be older than every value shown
> here, and a horizon can be held back by a value that no row reports.
> An invalidated slot holds back nothing and reports null in all three
> columns.

14. I rewrote this part. The server keeps two combined slot values,
one for xmin and one for catalog_xmin, and they feed different horizons.

> This one is hard to follow. How about a short example query
> instead of describing the sort in prose, as in comment 3, and
> splitting the rest so each paragraph does one thing?
>
> To find the row holding back an xmin horizon, use a query such as:
> << query here >>
> xid has no ordering operators, so the sort uses age(), which is
> wraparound-safe. Which rows apply depends on the column and on datid,
> as described above.
>
> When several rows report the same oldest value, that xmin horizon
> cannot advance until all of them release it, so resolving only one of
> them has no effect. If one of those rows has an xid equal to that
> value, the horizon has to wait for that transaction to finish.
>
> When no row reports a value for an xmin horizon, it advances on its
> own to a point just past the newest completed transaction, which this
> view does not show. Note also that backend rows and replication slot
> rows are gathered at slightly different times, so a single query is
> not a snapshot of the whole cluster at one instant. See <xref
> linkend="vacuum-for-wraparound"/> for the steps to take when an xmin
> horizon is held back.

15. I split the paragraph but declined the example query for the same
reason as 2.

> How about "Querying pg_xmin_horizon on a standby is not supported
> and raises an error, because the view cannot report the primary's
> running transactions during recovery"?

16. I changed this to match other recovery restrictions like those in
func-admin.sgml.

> How about defining a macro for this flag test similar to PROC_*
> and using it in all three places, instead of a comment asking future
> authors to keep them in sync?

17. Done.

> GetXidHorizonProcs() acquires ProcArrayLock once and gathers all
> procs in a single pass, whereas pg_stat_activity takes the lock once
> per backend, so each snapshot does N acquisitions and releases. I
> measured this by keeping ~500 backends connected with pgbench, then
> running each view repeatedly from one backend while a separate
> 4-client pgbench write workload ran alongside. Query exec time is
> 0.304 ms vs 1.335 ms for pg_xmin_horizon and pg_stat_activity
> respectively (4.4x), and the write workload TPS reduces by 11.7% vs
> 27.4%. Connection latency is also lower since connection setup takes
> the lock exclusive, measuring 2.21 ms with no polling, 2.55 ms with
> pg_xmin_horizon polling, and 3.09 ms with pg_stat_activity polling.

18. Thanks.

> The comment mostly says what the exact code does incl. variable
> names, and the one thing it adds, that the rows are mutually
> consistent, is not what the shared lock gives. How about the
> following?
>
> /*
> * Get the xid, xmin and other horizon inputs of every proc, in one pass over
> * the proc array.
> *
> * These are the same per-proc inputs that ComputeXidHorizons() uses. Processes
> * cannot enter or leave the array during the pass, but a process can install a
> * new xmin or get an xid while the pass is still running, see
> * GetSnapshotData() and GetNewTransactionId(), so the values returned can be
> * from different moments.
> *
> * There is a race here. The lock is released before returning, so by the time
> * the caller reads an entry the process may have ended its transaction or
> * exited altogether.
> *
> * The caller is responsible for freeing the returned array.
> */

19. I rewrote the comment. The property that you called a "race"
applies to any function that returns data that may change later, and it
doesn't warrant a separate paragraph.

> How about returning both the array and its length as out
> parameters, void GetXidHorizonProcs(XidHorizonProc **procs, int
> *nprocs)?

20. GetXidHorizonProcs() follows the convention of
GetVirtualXIDsDelayingChkpt() and GetCurrentVirtualXIDs() in the same
file.

> The count variable looks redundant. Nothing is skipped in the
> loop, so count always ends up equal to arrayP->numProcs, which could
> be used directly.

21. Done.

> With pgstat_get_beentry_by_proc_number(), the new function is not
> needed unless I'm missing something else. It does the change count
> retry internally, so something like the following is enough.
>
> PgBackendStatus *beentry = pgstat_get_beentry_by_proc_number(p->procNumber);
>
> if (beentry != NULL && beentry->st_procpid == pid &&
> beentry->st_backendType != B_WAL_SENDER)
> r->xact_start = beentry->st_xact_start_timestamp;

22. As Surya pointed out, pgstat_get_beentry_by_proc_number() reads from
a cache, and pgstat_get_xact_start_by_proc_number() exists to read fresh
data.

> Nit: How about using consistent wording here?
> int pid; /* Backend's process ID; 0 if prepared xact */
> int procNumber; /* ProcNumber, which is also the
> * BackendStatusArray index; not valid for
> * prepared xact dummy procs */

23. Done.

> Nit: proisstrict has no effect for a function with no arguments,
> and pg_get_aios(), pg_lock_status() and pg_prepared_xact() all leave
> it out. How about dropping it? The descr could also read like its
> neighbours, something such as 'information about sources holding back
> the cluster's xmin horizons'.

24. Done.

> How about merging the second and third passes in
> pg_get_xmin_horizon(), something like the attached diff on top of v6?
> Emitting rows while holding ReplicationSlotControlLock seems okay, the
> same way pg_get_replication_slots() already does it. Is there a reason
> not to do this?

25. pg_get_xmin_horizon_build_tuple() declares 10 parameters, and 5
consecutive parameters use uint32 after resolving typedefs. A developer
could easily pass them in the wrong order.

> Nit: How about something like the following to be clearer?
>
> * procarrayfuncs.c
> * SQL-callable functions related to POSTGRES process array code.
> *

26. I rewrote the comment to follow lockfuncs.c and waitfuncs.c.

> For a parallel query, and for concurrent REPACK with decoding
> workers, the leader and every worker gets its own pinning row. Nothing
> in the view itself links a worker row back to its leader, though
> joining pg_stat_activity on pid gives leader_pid for anyone who wants
> it. How about a doc note saying so?

27. Done.

> Decoding through pg_logical_slot_get_changes() shows up as an
> ordinary backend row with an xmin of its own, which is worth knowing
> for anyone looking at what holds the horizon back. How about a doc
> note saying so?

28. Done.

> The note in the docs about a backend running VACUUM showing null
> in shared_xmin, catalog_xmin and data_xmin holds only for lazy VACUUM,
> which is the one path that sets PROC_IN_VACUUM. VACUUM FULL and
> CLUSTER report an xid and all three xmins, and ANALYZE reports all
> three xmins with a null xid. How about clarifying this in the docs?
>
> statement xid shared_xmin catalog_xmin data_xmin
> lazy VACUUM null null, null, null
> VACUUM FULL 668 668, 668, 668
> CLUSTER 681 681, 681, 681
> ANALYZE null 681, 681, 681

29. I clarified this part. CLUSTER needs no special mention.

The attached v7 applies to 3c5d9d914fa.

--
Scott Ray