Re: snapshot too old issues, first around wraparound and then more.

Hi,

On 2020-04-01 17:54:06 -0700, Andres Freund wrote:
> * Check whether the given snapshot is too old to have safely read the given
> * page from the given table. If so, throw a "snapshot too old" error.
> *
> * This test generally needs to be performed after every BufferGetPage() call
> * that is executed as part of a scan. It is not needed for calls made for
> * modifying the page (for example, to position to the right place to insert a
> * new index tuple or for vacuuming). It may also be omitted where calls to
> * lower-level functions will have already performed the test.

To me this sounds like we'd not need to check for an old snapshot in
heap_delete/update/lock_tuple. And they were explictly not testing for
old snapshots. But I don't understand why that'd be correct?

In a lot of UPDATE/DELETE queries there's no danger that the target
tuple will be pruned away, because the underlying scan node will hold a
pin. But I don't think that's guaranteed. E.g. if a tidscan is below the
ModifyTable node, it will not hold a pin by the time we heap_update,
because there's no scan holding a pin, and the slot will have been
materialized before updating.

There are number of other ways, I think.

So it's possible to get to heap_update/delete (and probably lock_tuple
as well) with a tid that's already been pruned away. Neither contains a
non-assert check ensuring the tid still is normal.

With assertions we'd fail with an assertion in PageGetItem(). But
without it looks like we'll interpret the page header as a tuple. Which
can't be good.

Greetings,

Andres Freund