| From: | Scott Ray <scott(at)scottray(dot)io> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, surya poondla <suryapoondla4(at)gmail(dot)com> |
| Subject: | Re: pg_xmin_horizon: a system view of everything pinning the xmin horizon |
| Date: | 2026-09-05 03:25:05 |
| Message-ID: | 3TmV7r48RekS9lI-goS5ZDrVOfRrrNwFY1nnghf05L3Ays6_km2Qu0LWqCg_-LhUJrUbKjhpm9VI6pjB7aZTpIMEe_9kAkpL_j4Dj08KazY=@scottray.io |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Bharath,
Thanks for the comments.
On Monday, August 31st, 2026 at 8:15 PM, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | shihao zhong | 2026-09-05 03:36:43 | Re: logical decoding: skip unnecessary snapshot distribution. |
| Previous Message | shihao zhong | 2026-09-05 03:24:25 | Re: pg_createsubscriber: allow duplicate subscription names |