Re: old_snapshot_threshold allows heap:toast disagreement

On 2016-07-28 15:40:48 -0400, Robert Haas wrote:
> On Wed, Jul 27, 2016 at 7:26 PM, Andres Freund <andres@anarazel.de> wrote:
> >> Also, I wonder why it's right to use
> >> pairingheap_first() instead of looking at the oldest snapshot on the
> >> active snapshot stack, or conversely why that is right and this is
> >> not.  Or maybe we need to check both.
> >
> > Well, generally, the older the snapshot we use is, the more we'll error
> > out spuriously. The reason to use the oldest from the pairing heap is
> > that that'll be the most conservative value, right?  If there's neither
> > an active, nor a registered snapshot, we'll not prevent pruning in the
> > first place (and xmin ought to be invalid).  As registered snapshots are
> > usually "older" than active ones, we definitely have to check those. But
> > you're right, it seems safer to also check active ones - which squares
> > with SnapshotResetXmin().
> 
> OK.  That's a bit inconvenient because we don't have an O(1) way to
> access the bottom of the active snapshot stack; I've attempted to add
> such a mechanism here.

I think just iterating through the active snapshots would have been
fine. Afaics there's no guarantee that the first active snapshot pushed
is the relevant one - in contrast to registered one, which are ordered
by virtue of the heap.


> >> But there's a bit of spooky action at a
> >> distance: if we don't see any snapshots, then we have to assume that
> >> the scan is being performed with a non-MVCC snapshot and therefore we
> >> don't need to worry.  But there's no way to verify that via an
> >> assertion, because the connection between the relevant MVCC snapshot
> >> and the corresponding TOAST snapshot is pretty indirect, mediated only
> >> by snapmgr.c.
> >
> > Hm. Could we perhaps assert that the session has a valid xmin?
> 
> I don't think so.  CLUSTER?

That should have one during any toast lookups afaics - the relevant code
is        /* Start a new transaction for each relation. */        StartTransactionCommand();        /* functions in
indexesmay want a snapshot set */        PushActiveSnapshot(GetTransactionSnapshot());        /* Do the job. */
cluster_rel(rvtc->tableOid,rvtc->indexOid, true, stmt->verbose);        PopActiveSnapshot();
CommitTransactionCommand();
right? And
Snapshot
GetSnapshotData(Snapshot snapshot)
{
...
if (!TransactionIdIsValid(MyPgXact->xmin))    MyPgXact->xmin = TransactionXmin = xmin;
sets xmin.


Greetings,

Andres Freund