| From: | surya poondla <suryapoondla4(at)gmail(dot)com> |
|---|---|
| To: | scott(at)scottray(dot)io |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, solai v <solai(dot)cdac(at)gmail(dot)com> |
| Subject: | Re: pg_xmin_horizon: a system view of everything pinning the xmin horizon |
| Date: | 2026-08-18 23:38:37 |
| Message-ID: | CAOVWO5p8boS_gcjVTJs8A=VkC_uz1OL0KjeZh6SBeUqsfbHzzg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Scott,
+1 on the feature.
This is a nice thread with some good review comments.
I applied v4, it builds clean with no new warnings.
A few comments:
1. maintenance.sgml and system-views.sgml contradict each other on datid,
and I feel maintenance.sgml is wrong. It says to consider "only
rows whose datid is the affected database or null", but
ReplicationSlotsComputeRequiredXmin() aggregates catalog_xmin over
every in-use, non-invalidated slot with no filter on s->data.database, and
that folds into the shared and
catalog horizons unconditionally. A logical slot on database A is reported
with datid = A, so it can hold back database B's catalog horizon.
system-views.sgml says the opposite and is right: "Slot and
standby_feedback rows pin the horizon cluster-wide regardless of
datid, and a query filtering by datid will exclude them.", maybe we can fix
this.
2. Would a column distinguishing the xid holder from xmin holders be worth
adding?
Since the SRF folds p->xid and p->xmin into one value, a backend that owns
the oldest xid and a backend that merely snapshotted it come out
identical. For example, one idle-in-transaction backend owning the oldest
xid, with twenty other backends whose backend_xmin equals it, produces
twenty-one indistinguishable rows and system-views.sgml then says "When
several rows tie for the oldest value, removing one of them does not
advance the horizon", which I think inverts the advice for that case, since
terminating the single xid owner is what actually helps.
To be clear, I agree with your argument on the other thread that the view
should list every input rather than nominate one.
My only concern is that the role isn't recorded anywhere, so the fact a DBA
most needs is the one the view can't convey.
3. It would be worth adding a test for the case where a slot's
effective_xmin is set while data.xmin is still invalid, since that's
the state pg_replication_slots cannot show. TAP case (3) asserts a logical
slot with catalog_xmin non-null and data_xmin NULL, but
pg_replication_slots reports the same for that slot.
To reach the divergent state we need CREATE_REPLICATION_SLOT ... LOGICAL
<plugin> (SNAPSHOT 'export') over a replication connection;
pg_create_logical_replication_slot() passes need_full_snapshot = false, so
it never pegs effective_xmin.
Conveniently the state is stable and ReplicationSlotRelease() only clears
effective_xmin when the slot is released, so another session can
compare the two views while the replication connection is held open.
A few minor things:
- The recovery errhint() puts the function name inside the format string.
We can use a placeholder to keep the name
untranslated, like how pg_log_standby_snapshot() does.
- The xact_start assertion in the regression test compares the view's
xact_start against pg_stat_activity.xact_start, which is where the view
sources it, so it exercises the join rather than the value.
Regards,
Surya Poondla
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Jeff Davis | 2026-08-19 00:05:29 | Fix for fragile code in ltree/crc32.c |
| Previous Message | Kevin Rocker | 2026-08-18 22:42:31 | [Patch] Make ginInsertCleanup follow vacuum's BufferAccessStrategy |