Re: MVCC catalog access

On Mon, Jul 1, 2013 at 10:04 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> This is really cool stuff.

Thanks.

> I have to say, if the thing that primarily suffers is pretty extreme DDL
> in extreme situations I am not really worried. Anybody running anything
> close to the territory of such concurrency won't perform that much DDL.

/me wipes brow.

> Something picked up when quickly scanning over the last version of the
> patch:
>
>> +/*
>> + * Staleness detection for CatalogSnapshot.
>> + */
>> +static bool CatalogSnapshotStale = true;
>>
>> /*
>> * These are updated by GetSnapshotData. We initialize them this way
>> @@ -177,6 +188,9 @@ GetTransactionSnapshot(void)
>> else
>> CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
>>
>> + /* Don't allow catalog snapshot to be older than xact snapshot. */
>> + CatalogSnapshotStale = true;
>> +
>> FirstSnapshotSet = true;
>> return CurrentSnapshot;
>> }
>> @@ -184,6 +198,9 @@ GetTransactionSnapshot(void)
>> if (IsolationUsesXactSnapshot())
>> return CurrentSnapshot;
>>
>> + /* Don't allow catalog snapshot to be older than xact snapshot. */
>> + CatalogSnapshotStale = true;
>> +
>> CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
>>
>> return CurrentSnapshot;
>> @@ -207,6 +224,54 @@ GetLatestSnapshot(void)
>> }
>
> Do we really need to invalidate snapshots in either situation? Isn't it
> implied, that if it's still valid, according to a) no invalidation via local
> invalidation messages b) no invalidations from other backends, there
> shouldn't be any possible differences when you only look at the catalog?

I had the same thought, removed that code, and then put it back. The
problem is that if we revive an older snapshot "from the dead", so to
speak, our backend's advertised xmin might need to go backwards, and
that seems unsafe - e.g. suppose another backend has updated a tuple
but not yet committed. We don't see any invalidation messages so
decide reuse our existing (old) snapshot and begin a scan. After
we've looked at the page containing the new tuple (and decided not to
see it), vacuum nukes the old tuple (which we then also don't see).
Bad things ensue. It might be possible to avoid the majority of
problems in this area via an appropriate set of grotty hacks, but I
don't want to go there.

> And if it needs to change, we could copy the newly generated snapshot
> to the catalog snapshot if it's currently valid.

Yeah, I think there's room for further fine-tuning there. But I think
it would make sense to push the patch at this point, and then if we
find cases that can be further improved, or things that it breaks, we
can fix them. This area is complicated enough that I wouldn't be
horribly surprised if we end up having to fix a few more problem cases
or even revert the whole thing, but I think we've probably reached the
point where further review has less value than getting the code out
there in front of more people and seeing where (if anywhere) the
wheels come off out in the wild.

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