Re: old_snapshot_threshold allows heap:toast disagreement - Mailing list pgsql-hackers

On Thu, Jul 28, 2016 at 7:29 PM, Andres Freund <andres@anarazel.de> wrote:
> 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.

After a bit of scrutiny, I don't think this is a problem, and I don't
want to introduce belt-and-suspenders protections against can't-happen
conditions that may get cargo-culted into other places.  (How's that
for cramming as many Tom-Lane-isms as possible into one sentence?
More might be done, but it would be some sort of bespoke crock of the
first water.)

Anyway, the reason I think it's OK is that PushActiveSnapshot() does
not copy the input snapshot unless it's CurrentSnapshot or
SecondarySnapshot -- so every snapshot that gets pushed on the active
snapshot stack is either one that's already registered or one that was
just taken (because CurrentSnapshot will get overwritten on next call
to GetTransactionSnapshot).  In the former case, it's included in the
registered snapshots so it doesn't even matter whether we notice that
it is also on the active snapshot stack.  In the latter case, having
just been taken, it must be newer than the oldest registered snapshot,
which was necessarily taken earlier.  I think that the only time it
matters whether we even look at the active snapshot stack is when
there are no registered snapshots whatsoever.  In many cases -- like
QueryDescs -- we register the snapshot first and then later put it on
the active snapshot stack, but there are some things like CLUSTER that
only make the snapshot active and don't register it.  But if they do
that, they can only do it in first-in, first-out fashion.

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

Yeah.  Actually, consistent with the above, I discovered that as long
as we consult both the active snapshot stack and the pairingheap of
registered snapshots, it seems to be fine to just insist that we
always get a snapshot back from the snapmgr and use that to initialize
the TOAST snapshot.  So I did it that way in the attached version.

New patch attached.  Barring objections, I will commit this tomorrow.
If there are objections, I will send another status update tomorrow.

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