Hi Scott,
I reviewed the v6 and have some comments.
pgstat_get_xact_start_by_proc_number() reads BackendStatusArray directly, so the value is no longer taken from a cached snapshot that could predate the procarray pass by an arbitrary amount.
The residual race you document in the header comment is a much smaller window, and the change-count retry loop and the pid <= 0 guard both look right. I also checked that p->procNumber = pgprocno is the right ProcNumber for indexing the status array, and that a prepared xact's dummy proc can't reach the array out of bounds, the pid <= 0 early return catches it before the bounds check does.
The maintenance.sgml rewrite is better than what I suggested. Sorting all rows by age(shared_xmin) DESC NULLS LAST, and explaining that
datfrozenxid can never advance past the shared horizon and that every ow's shared_xmin feeds it including backends in other databases, avoids the datid scoping question entirely rather than trying to describe it.
The datfrozenxid clarification is a good catch too.
One question. pg_stat_get_activity() gates st_xact_start_timestamp behind HAS_PGSTAT_PERMISSIONS, but the new helper applies no such check.
That isn't reachable today, since both the view and the SRF require pg_read_all_stats, and such a caller already passes that test for every backend.
But it does mean the SRF omits a check pg_stat_activity applies. Bharath pointed out one asymmetry already i.e pg_stat_activity shows pid, datid, xid and xmin to unprivileged non-owners while
pg_xmin_horizon shows nothing, and this would be one in the other direction if the ACL were ever loosened or EXECUTE granted directly.
Would it be worth applying HAS_PGSTAT_PERMISSIONS per row in the SRF?
Regards,
Surya Poondla
