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

Hi,

On 2020-04-15 17:56:58 -0400, Alvaro Herrera wrote:
> 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.

Fair enough. I just looked far enough to find where the comment was
introduced. But I'm not sure the logic leading to that is correct - see
below.

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

Yea. I think there's a lot of very old sloppiness around xmin horizons
that we just avoided hitting frequently due to what you say. Although
I'm fairly sure that we've declared some of the resulting bugs
OS/hardware level issues that actually were horizon related...

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

I left out the "typically need a snapshot" part because it doesn't
really seem to say something meaningful. To me it's adding confusion
without removing any. I guess we could reformulate it to explain that
while we don't use the snapshot to make mvcc visibility determinations
(since only definitely dead rows matter to vacuum), nor do we use it to
prevent concurrent removal of dead tuples (since we're fine with that
happening), we still need to ensure that resources like pg_subtrans stay
around.

I left out the "in indexes" part because I didn't see it
saying/guaranteeing much. Thinking more about it, that's probably not
quite right. In fact, I wonder if PushActiveSnapshot() does anything
meaningful for the case of expression indexes. If a snapshot is needed
for some indexes, I assume that would be because visibility tests are
done. But because we set PROC_IN_VACUUM it'd not at all be safe to
actually do visibility tests - rows that are still visible could get
removed.

It's not clear to me which functions this is talking about. For vacuum
full, where the comment originated from, it's obvious (new index being
built) that we need to evaluate arbitrary functions, in particular for
expression indexes. But there's no different behaviour for expression
indexes during normal vacuums, all the expression related work was done
during index insertion. And if the index operators themselves needed a
valid snapshot, it'd not be safe to set PROC_IN_VACUUM.

I think, at least for btree, it's presumably ok that we invoke btree
operators without a valid snapshot. Because we need to be able to
compare tuples on inner pages during normal operation, which might long
ago may have been removed, btree operators need to be safe against
underlying data vanishing anyway (which is why e.g. enum values are hard
to remove).

Greetings,

Andres Freund