Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On Wed, Jul 20, 2016 at 12:30 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> And how do you obtain that? The functions that reference
>> SnapshotToast are toast_delete_datum, toastrel_value_exists, and
>> toast_fetch_datum, toast_fetch_datum_slice, but none of those take a
>> snapshot as an argument, nor is there any reasonable way to make them
>> do so. Those are indirectly called by things like bttextcmp, which
>> don't know what snapshot was used to fetch the datum that they are
>> detoasting and can't reasonably be made to know.
>>
>> I mean, you could do something *approximately* correct by calling
>> GetActiveSnapshot() but that doesn't seem likely to be correct in
>> detail.
>
> GetActiveSnapshot() seems like it should work well enough in this case,
> or we could use pairingheap_first() to get the actual oldest registered
> one.

It's hard to believe that it's equally good to use the newest
registered snapshot (which is, I think, what you will often get from
GetActiveSnapshot()) and the oldest registered snapshot (which is what
you will get from pairingheap_first()). It seems to me that we need
to analyze what happens if we choose a snapshot that is older than the
one used to find the datum which contained the toast pointer, and
conversely what happens if we use a snapshot that is newer than the
one we used to find the toast pointer.

Here's an attempt:

1. If we pick a snapshot that is older than the one that found the
scan tuple, we might get a "snapshot too old" error that is not
strictly necessary.

2. If we pick a snapshot that is newer than the one that found the
scan tuple, then haven't we failed to fix the problem? I'm not so
sure about this direction, but if it's OK to test an arbitrarily new
snapshot, then I can't see why we need the test at all.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company