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

Hi,

> > The need for this comment seems like a sign that this logic is happening at
> > the wrong level.
>
> Master already tests those two flags in ComputeXidHorizons() and
> GetSnapshotData(). The tree has dozens of comments that say "keep in
> sync". They mark dependencies, not misplaced logic. v7 replaces the
> test you quoted with one macro used at all three sites. One
> keep-in-sync comment remains where ComputeXidHorizons() applies the slot
> xmins.

After thinking about this some more, I think the right level for the
shared part is the small procarray rules next to ComputeXidHorizons(),
rather than a new procarrayfuncs.c file.

The idea I have been playing with locally is to add a
ComputeXidHorizonsData() helper next to ComputeXidHorizons(). It
doesn't call ComputeXidHorizons() directly, because the reporting side
needs source attribution and all-database reporting, as you mention.
But it can share the parts that are easy to get wrong, like how we
derive the effective proc xmin from xid/xmin, and which procs don't
affect removable-tuple horizons.

So ComputeXidHorizons() stays the same, except for using those small
helpers. ComputeXidHorizonsData() is a simpler procarray collector. It
returns latestCompletedXid + 1, the recovery KnownAssignedXids xmin, and
PGPROC entries that can affect horizons, with their classification
(backend, prepared transaction, or standby feedback), database OID, and
effective xmin. The SQL function can then combine that with the slot
data it already has to inspect separately to build the reported horizon
rows. This still duplicates some slot horizon handling for now, but
avoids having the SQL-facing code independently interpret the procarray
rules.

Initially, I thought about a ComputeXidHorizonsInternal() which takes a
bool called with_horizon_data and returns the data needed by the SQL
function. ComputeXidHorizons() would then become a wrapper for
ComputeXidHorizonsInternal() with with_horizon_data = false. I did not
have any performance concerns with this approach, but I also no longer
like the idea of turning ComputeXidHorizons() into a multi-purpose
function.

> > The view contains the raw information needed to answer these questions, but
> > leaves the DBA to reconstruct the effective horizons in SQL.
>
> A query over the patch's rows produces the summary. No query turns the
> summary back into the rows. Andres asked for "a view showing all the
> sources of the horizon being held back" [1]. The view shows every
> source and the gaps between them, which tell the user how far the
> horizons could advance.

I did not read "sources" here as necessarily meaning one row per PID. I
went digging into older conversations and found [2], where Andres lays
out a potential shape for this view, where we key every row by "datname"
and "horizon", and where the shared horizon itself is a NULL datname.

So I think we can start out with a function like this which just returns
the data for each datid/horizon, such as:

```
postgres=# select * from pg_get_vacuum_horizons();
-[ RECORD 1 ]-----------------+--------
horizon | shared
datid |
xid | 664
source_count | 2
backend_horizon | 664
prepared_xact_horizon |
physical_slot_horizon |
physical_slot_catalog_horizon |
logical_slot_horizon |
logical_slot_catalog_horizon |
standby_feedback_horizon |
primary_xact_horizon |
-[ RECORD 2 ]-----------------+--------
horizon | catalog
datid | 1
xid | 666
...
.....
-[ RECORD 3 ]-----------------+--------
horizon | data
datid | 1
xid | 666
source_count | 0
...
.....
-[ RECORD 4 ]-----------------+--------
horizon | catalog
datid | 4
xid | 666
source_count | 0
...
.....
-[ RECORD 5 ]-----------------+--------
horizon | data
datid | 4
xid | 666
source_count | 0
...
.....
-[ RECORD 6 ]-----------------+--------
horizon | catalog
datid | 5
xid | 664
source_count | 1
backend_horizon | 664
...
.....
-[ RECORD 7 ]-----------------+--------
horizon | data
datid | 5
xid | 664
source_count | 1
backend_horizon | 664
...
.....
-[ RECORD 8 ]-----------------+--------
horizon | catalog
datid | 16384
xid | 666
source_count | 1
backend_horizon | 666
...
.....
-[ RECORD 9 ]-----------------+--------
horizon | data
datid | 16384
xid | 666
source_count | 1
backend_horizon | 666
...
.....
```

> > Would it make sense to drive the view from ComputeXidHorizons() and add the
> > source attribution on top of those results?
>
> Do you mean calling ComputeXidHorizons() for the values and finding
> the sources in a second pass, or changing ComputeXidHorizons() to
> record them?

And using the base function, we can construct a system view, or document
a way to use it with joins on other system views to come up with a view
that looks like:

```
horizon | datname | xid | source_count | source_type | pid |
slot_name | gid | source_xmin
---------+----------+-----+--------------+-------------+--------+-----------+-----+-------------
shared | | 664 | 2 | backend | 330161 |
| | 664
catalog | postgres | 664 | 1 | backend | 330161 |
| | 664
catalog | db1 | 666 | 1 | backend | 330353 |
| | 666
data | postgres | 664 | 1 | backend | 330161 |
| | 664
data | db1 | 666 | 1 | backend | 330353 |
| | 666
(5 rows)
```

horizon | datname | xid | source_count | source_type | pid |
slot_name | gid | source_xmin
---------+----------+-----+--------------+-------------+--------+-----------+-----+-------------
shared | | 664 | 2 | backend | 330161 |
| | 664
catalog | postgres | 664 | 1 | backend | 330161 |
| | 664
catalog | db1 | 666 | 1 | backend | 330353 |
| | 666
data | postgres | 664 | 1 | backend | 330161 |
| | 664
data | db1 | 666 | 1 | backend | 330353 |
| | 666
(5 rows)
```

What I like about something like this is that it is condensed and easy
for a DBA to act on. They get the global horizon in the "shared" row,
and they also get the per-database xmin horizons. source_count tells
the DBA when there is more than one potential blocker, but we show the
oldest one that should be acted on first. So I wonder if this is a
better starting point than exposing one row per source, while keeping
the source-attribution details simpler for the first version?

If someone wants to expand this into all matching sources, they can
write more complex queries on top of the base function. But for a view
that is meant to provide actionable information to a DBA, I think this
shape is much easier to use.

The VACUUM logging angle [3] also needs to be considered, but not for
this patch. IMO, getting this view right is more important than VACUUM
logging for now. But ComputeXidHorizonsData() could in the future be
taught to report for one database only, so we can identify blockers for
the database being vacuumed.

[1] https://wiki.postgresql.org/wiki/User:Andresfreund/Desired_Changes
[2] https://www.postgresql.org/message-id/20231026174136.4et3ktuegmtlgxfs@awork3.anarazel.de
[3] https://www.postgresql.org/message-id/CAOzEurSgy-gDtwFmEbj5+R9PL0_G3qYB6nnzJtNStyuf87VSVg@mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)