Comment explaining why vacuum needs to push snapshot seems insufficient.

Hi,

vacuum_rel() has the following comment:
/*
* Functions in indexes may want a snapshot set. Also, setting a snapshot
* ensures that RecentGlobalXmin is kept truly recent.
*/
PushActiveSnapshot(GetTransactionSnapshot());

which was added quite a while ago in

commit d53a56687f3d4772d17ffa0013a33231b7163731
Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Date: 2008-09-11 14:01:10 +0000

But to me that's understating the issue. Don't we e.g. need a snapshot
to ensure that pg_subtrans won't be truncated away? I thought xlog.c
doesn't pass PROCARRAY_FLAGS_VACUUM to GetOldestXmin when truncating?
TruncateSUBTRANS(GetOldestXmin(NULL, PROCARRAY_FLAGS_DEFAULT));

It's fine for rows that vacuum could see according to its xmin to be
pruned away, since that won't happen while it has a page locked. But we
can't just have a pg_subtrans access error out, and there's no page
level interlock against that.

Also, without an xmin set, it seems possible that vacuum could see some
of the transaction ids it uses (in local memory) wrap around. While not
likely, it doesn't seem that unlikely either, since autovacuum will be
running full throttle if there's a 2 billion xid old transaction hanging
around. And if that super old transaction finishes, e.g. vacuum's
OldestXmin value could end up being in the future in the middle of
vacuuming the table (if that table has a new relfrozenxid).

How about replacing it with something like
/*
* Need to acquire a snapshot to prevent pg_subtrans from being truncated,
* cutoff xids in local memory wrapping around, and to have updated xmin
* horizons.
*/

Greetings,

Andres Freund