Re: MVCC catalog access

On Thu, Jun 6, 2013 at 5:30 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> + * XXX: Now that we have MVCC catalog access, the reasoning above is no longer
>> + * true.  Are there other good reasons to hard-code this, or should we revisit
>> + * that decision?
>
> We could just the function by looking in the shared
> relmapper. Everything that can be mapped via it is shared.

I suspect there are several possible sources for this information, but
it's hard to beat a hard-coded list for efficiency, so I wasn't sure
if we should tinker with this or not.

>> --- a/src/backend/commands/cluster.c
>> +++ b/src/backend/commands/cluster.c
>> @@ -480,6 +480,11 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
>>   * against concurrent SnapshotNow scans of pg_index.  Therefore this is unsafe
>>   * to execute with less than full exclusive lock on the parent table;
>>   * otherwise concurrent executions of RelationGetIndexList could miss indexes.
>> + *
>> + * XXX: Now that we have MVCC catalog access, SnapshotNow scans of pg_index
>> + * shouldn't be common enough to worry about.  The above comment needs
>> + * to be updated, and it may be possible to simplify the logic here in other
>> + * ways also.
>>   */
>
> You're right, the comment needs to be changed, but I don't think the
> effect can. A non-inplace upgrade changes the xmin of the row which is
> relevant for indcheckxmin.

OK.

> (In fact, isn't this update possibly causing problems like delaying the
> use of such an index already)

Well, maybe.  In general, the ephemeral snapshot taken for a catalog
scan can't be any older than the primary snapshot already held.  But
there could be some corner case where that's not true, if we use this
technique somewhere that such a snapshot hasn't already been acquired.

> Hm. Looks like this should also change the description of SecondarySnapshot:
>
> /*
>  * CurrentSnapshot points to the only snapshot taken in transaction-snapshot
>  * mode, and to the latest one taken in a read-committed transaction.
>  * SecondarySnapshot is a snapshot that's always up-to-date as of the current
>  * instant, even in transaction-snapshot mode.  It should only be used for
>  * special-purpose code (say, RI checking.)
>  *

I think that's still more or less true, though we could add catalog
scans as another example.

> and
>         /*
>          * Checking SecondarySnapshot is probably useless here, but it seems
>          * better to be sure.
>          */

Yeah, that is not useless any more, for sure.

> Also looks like BuildEventTriggerCache() in evtcache.c should use
> GetInstanSnapshot() now.

OK.

> I actually wonder if we shouldn't just abolish GetLatestSnapshot(). None
> of the callers seem to rely on it's behaviour from a quick look and it
> seems rather confusing to have both.

I assume Tom had some reason for making GetLatestSnapshot() behave the
way it does, so I refrained from doing that.  I might be wrong.

> I think we need another term for what SnapshotNow used to express
> here... Imo this description got less clear with this change.

I thought it was OK but I'm open to suggestions.

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