Re: MVCC catalog access

On 2013-06-06 12:49:14 -0400, Robert Haas wrote:
> 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.

I can tell from experience that it makes adding a new shared relation
more of a pain than it already is, but we're hopefully not doing that
all that often.
I just don't think that the mvcc angle has much to do with the decision.

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

I wasn't talking about catalog scans or this patch, I wonder whether the
update we do there won't cause the index not being used for concurrent
(normal) scans since now the xmin is newer while it might be far in the
past before. I.e. we might need to unset indexcheckxmin if xmin is far
enough in the past.

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

I guess my feeling is that once catalog scans use it, it's not so much
special purpose anymore ;). But I admit that the frequency of usage
doesn't say much about its specificity...

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

At least I don't see any on a quick look - which doesn't say very
much. I think I just dislike having *instant and *latest functions in
there, seems to be confusing to me.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services