Re: Comment explaining why vacuum needs to push snapshot seems insufficient.

On 2020-Apr-05, Andres Freund wrote:

> 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

Note that what that commit did was change the snapshot acquisition from
occurring solely during vacuum full to being acquired always -- and
added a small additional bit of knowledge:

- if (vacstmt->full)
- {
- /* functions in indexes may want a snapshot set */
- PushActiveSnapshot(GetTransactionSnapshot());
- }
- else
+ /*
+ * Functions in indexes may want a snapshot set. Also, setting
+ * a snapshot ensures that RecentGlobalXmin is kept truly recent.
+ */
+ PushActiveSnapshot(GetTransactionSnapshot());

so I wouldn't blame that commit for failing to understand all the side
effects of acquiring a snapshot there and then.

> 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));

Nice find. This bug would probably be orders-of-magnitude easier to hit
now than it was in 2008 -- given both the hardware advances and the
increased transaction rates.

> Also, without an xmin set, it seems possible that vacuum could see some
> of the transaction ids it uses (in local memory) wrap around.

Ditto.

> 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.
> */

"While we don't typically need a snapshot, we need the side effects of
acquiring one: having an xmin prevents concurrent pg_subtrans truncation
and prevents our cutoff Xids from becoming wrapped-around; this also
updates our Xmin horizons. Lastly, functions in indexes may want a
snapshot set."

(You omitted that last point without explanation -- maybe on purpose?
is it no longer needed?)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services