Hi Bharath, Thanks for the comments.
On Monday, August 31st, 2026 at 8:15 PM, Bharath Rupireddy <[email protected]> wrote: > I think having pg_xmin_horizon report similar information on > the standby, say, the long-running transaction or long-running > analytical query or replication slot, for example, could be a valuable > addition and will be useful in many situations. I agree, and in my first message in this thread, I suggested adding standby support in a follow-up patch. > There seems to be some degree of information > bloat being added by this patch. I mean, most of the columns are > already derivable from elsewhere today. We must try to address this > point. This may be okay given we compute things in one pass of > proc-array/replication slots. Let's see what others have to say about > this. Please explain specifically where you see information bloat. > 2/ pg_xmin_horizon needs to have similar checks as > HAS_PGSTAT_PERMISSIONS for disallowing roles that are not owners of > the sessions (unprivileged?) from unnecessarily scanning the > proc-array. I haven't spent enough time checking whether this can > happen with pg_read_all_stats predefined roles, but worth verifying. The view requires pg_read_all_stats or equivalent permissions, as I noted in my first email, the commit message, and the view documentation. src/test/regress/sql/xmin_horizon.sql verifies that accessing the view without the required permissions raises an error. The SRF requires the same. > 3/ The patch was around 1400+ LOC and hard to review. Could you try to > separate it out? > > 4/ I'm thinking of having a dedicated docs page, or as part of the > routine vacuuming page, a separate section covering all the possible > vacuum blockers that we know of so far (especially the two missing > ones: effective xmins and backends with special proc flags) that this > feature helps expose. This will be a good direction. I prefer to have > this as the 0001 patch. Next patches would each add one blocker kind: > 0002 transactions and tests, 0003 replication slots and tests, 0004 > prepared transactions and tests, 0005 standby support and tests. According to the wiki, "Normally changes should be submitted as a single patch that includes every file touched. If the patch is large and can be logically separated into distinct and separately commit-able sections for easier review, with a clear order they get applied in described when applicable, that can be more straightforward for reviewers to work with for more complicated patches." [1] First, this patch is not large by PG standards. Second, the feature contains no "separately commit-able sections", notwithstanding the division you proposed. Your patch 0002 would introduce pg_xmin_horizon, but the view would silently omit rows until at least 0004. The patch documents "the two missing ones" you mentioned and every holder class. For a broader reorganization of the vacuum documentation, I recommend starting a new thread. > And I > prefer to keep all the tests in one TAP test file rather than > spreading them across both SQL and TAP. I followed the guidance in src/test/perl/README, which says to prefer pg_regress when possible and reserve TAP for what pg_regress cannot express. > 5/ How about having a more impactful and user-readable name such as > pg_vacuum_blockers or similar instead of pg_xmin_horizon? The names of system views denote what they contain, and pg_xmin_horizon reports horizon inputs, including rows that block nothing, so pg_vacuum_blockers would misdescribe the contents. > 6/ It's also better to report the top N blockers (replication slot, > prepared transaction, long-running query, idle transaction, and so on) > in one pass of proc-array/replication slots. The view uses one pass over the procarray and another over the slot array. A user who wants "the top N blockers" can write ORDER BY age(shared_xmin) DESC NULLS LAST with a LIMIT. [1] https://wiki.postgresql.org/wiki/Submitting_a_Patch -- Scott Ray
signature.asc
Description: OpenPGP digital signature
