Re: Report oldest xmin source when autovacuum cannot remove tuples

On Mon, Jun 29, 2026 at 5:22 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:

> > if (kind != VISHORIZON_SHARED &&
> > + proc->databaseId != MyDatabaseId &&
> > + MyDatabaseId != InvalidOid &&
> > + !(statusFlags & PROC_AFFECTS_ALL_HORIZONS) &&
> > + !in_recovery)
> > + continue;
>
> First, although the comment says this mirrors the existing logic, it
> also adds extra conditions. I think it would be clearer to separate
> the common part from the function-specific part, so that the former
> remains almost a direct copy of the existing logic and the latter is
> handled separately.
>
> Related to that, I am worried that future changes to
> ComputeXidHorizons() could easily leave GetXidHorizonBlockers() out of
> sync. Would it be possible to factor the common decision logic out
> into a suitably named helper that both functions could use?

Thank you for the review. Agreed. In v8 I factored the per-database
filter into a new helper, ProcAffectsDataHorizon(), that both
ComputeXidHorizons() and GetXidHorizonBlockers() now call, so the
common logic is shared rather than duplicated and cannot drift apart.
The explanatory comment moved to the helper. The function-specific
exclusion (skipping the caller's own proc) is now a separate condition
with its own comment.

> Second, as far as I can tell, in_recovery is not a per-process
> condition, but rather controls whether the other filtering conditions
> are applied at all. Flattening those two concerns into a single
> condition makes the logic rather difficult to follow, particularly
> because the underlying filtering rules are already fairly involved. As
> written, it appears that recovery mode bypasses all of the other
> filtering conditions, so all processes are considered by the
> subsequent checks. Is that really the intended behavior?

Yes, considering all processes during recovery was intended, to mirror
ComputeXidHorizons(), which during recovery considers backends in all
databases for the data horizon because an accurate per-database
horizon cannot be computed then. But your next comment made me
reconsider whether GetXidHorizonBlockers() should handle recovery at
all.

> In addition, I am also not sure the recovery case is handled
> sufficiently here. As far as I understand, ComputeXidHorizons() does
> not rely only on the proc array during recovery; it also considers the
> oldest xmin from KnownAssignedXids. If the horizon is determined by
> that value, GetXidHorizonBlockers() may not be able to find a matching
> PGPROC entry. If this function is intended to explain the horizon
> computed by ComputeXidHorizons(), I think the recovery case needs
> either matching handling or an explicit comment explaining why it is
> safe to ignore that source.

You are right, the KnownAssignedXids case was not handled. The only
caller is VACUUM, which cannot run during recovery, so in v8 I removed
the recovery handling and replaced it with
Assert(!RecoveryInProgress()). A comment on the assertion notes that a
future caller running during recovery (for example a SQL-callable view
usable on a standby) must add recovery handling, including the
KnownAssignedXids source.

> Also, RecoveryInProgress() can change from true to false while the
> function is running. So I think it would be useful to explain
> somewhere why it is safe to keep using the saved value. I do not see
> such an explanation in ComputeXidHorizons() either, but this code made
> me think about it.

Now that the recovery handling is gone, the function no longer uses
RecoveryInProgress() outside the assertion, so your concern about
relying on the saved value no longer applies.

> > + if (TransactionIdEquals(proc_xid, horizon))
> > + {
> > + dst = &result[count++];
> ....
> > + dst->type = XHB_XMIN_ACTIVE_TRANSACTION;
> > + }
> > +
> > + if (dst)
> > + {
>
> This may just be a matter of taste, but I wonder whether this part
> could be written a bit more clearly. For example, the first
> if/else-if chain could determine only the blocker type, ending with an
> "else continue". Then the common work, including allocating dst, could
> be done afterwards in one place. I think that would make the flow a
> bit easier to follow.

Agreed. In v8 the chain determines only the blocker type and ends with
an else continue, and the entry is allocated and filled in one place
afterwards.

> One more thought about the allocation strategy here. The fixed-size
> "name" field seems to dominate the size of XidHorizonBlocker, but the
> name is filled only after releasing ProcArrayLock. So I wonder whether
> it needs to be embedded in every entry collected under the lock.
>
> If "name" were stored separately, for example as a char * allocated
> only when needed, the per-entry size would be much smaller. Then a
> single array of XidHorizonBlocker entries preallocated before taking
> ProcArrayLock would be less concerning. For example, with 1000
> entries, an entry of around 24 bytes would be about 24KB, and the
> strings would consume space only for entries that actually need one.
>
> Another possible approach would be to collect only the lock-protected
> fields in a small temporary array while holding ProcArrayLock, and
> then build the full result array after releasing the lock.

Since v6 the scan collects XidHorizonBlockerCandidate entries with no
name field (around 24 bytes each), and the name is resolved after all
locks are released, only for the single blocker that is actually
reported (see FillXidHorizonBlocker()). I believe this already matches
the second approach you describe. Please let me know if you meant
something more.

> > + active_proc = s->active_proc;
> > + invalidated = s->data.invalidated != RS_INVAL_NONE;
> > + SpinLockRelease(&s->mutex);
> > +
> > + /* Invalidated slots no longer hold back the horizon. */
> > + if (invalidated)
> > + continue;
>
> This is a very minor point, but I wonder whether it would be a bit
> cleaner to copy s->data.invalidated under the spinlock and perform the
> comparison afterwards, keeping the locked section limited to copying
> shared fields. I know there are other places that cache the boolean
> result directly, but unless the boolean value is going to be reused, I
> think comparing it afterwards reads a little more naturally.

Agreed, fixed in v8 to copy s->data.invalidated under the spinlock and
compare it against RS_INVAL_NONE afterwards.

--
Best regards,
Shinya Kato
NTT OSS Center