Re: Make relation_openrv atomic wrt DDL

On Thu, Jul 07, 2011 at 09:30:47PM -0400, Robert Haas wrote:
> On Thu, Jul 7, 2011 at 3:54 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> > Yes.  DDL-DDL concurrency is a much smaller practical concern, but it is a
> > quality-of-implementation hole.
>
> Agreed, although I'm not too pleased about the fact that this doesn't
> fix nextval(). That seems like a fairly significant hole (but one to
> be addressed by a later patch).

True.

> > The third sentence is true for all lock levels.  The fourth sentence is true
> > for lock levels _except_ NoLock.
>
> I rewrote this whole blurb. See what you think.

Seems better:

> + * DDL operations can change the results of a name lookup. Since all
> + * such operations will generate invalidation messages, we keep track
> + * of whether any such messages show up while we're performing the
> + * operation, and retry until either (1) no more invalidation messages
> + * show up or (2) the answer doesn't change.
> + *
> + * But if lockmode = NoLock, then we assume that either the caller is OK
> + * with the answer changing under them, or that they already hold some
> + * appropriate lock, and therefore return the first answer we get without
> + * checking for invalidation messages. Also, if the requested lock is
> + * already held, no LockRelationOid will not AcceptInvalidationMessages,

Extra word in that sentence.

> + * so we may fail to notice a change. We could protect against that case

I failed to note it last time, but it might be worth mentioning that failing to
notice a change only happens due to search_path interposition.

> + * by calling AcceptInvalidationMessages() before beginning this loop,
> + * but that would add a significant amount overhead, so for now we don't.

> >> +             /*
> >> +              * If no lock requested, we assume the caller knows what they're
> >> +              * doing.  They should have already acquired a heavyweight lock on
> >> +              * this relation earlier in the processing of this same statement,
> >> +              * so it wouldn't be appropriate to AcceptInvalidationMessages()
> >> +              * here, as that might pull the rug out from under them.
> >> +              */
> >
> > What sort of rug-pullout do you have in mind?  Also, I don't think it matters
> > if the caller acquired the lock during this _statement_.  It just needs to
> > hold a lock, somehow.
>
> What I'm specifically concerned about here is the possibility that we
> have code which does RangeVarGetRelid() multiple times and expects to
> get the same relation every time. Now, granted, any such places are
> bugs. But I am not eager to change the logic here without looking
> harder for them (and also for performance reasons).

Yeah, I think a key point is that we're not promising to avoid calling
AcceptInvalidationMessages() to prop up code that relies on it not getting
called. We just know it's not needed in this case, so we save that expense.

> > ... also making it cleaner to preserve this optimization.
>
> That optimization is now gone anyway.

Okay.

> > Incidentally, you've added in many places this pattern of commenting that a
> > lock level must match another lock level used elsewhere.  Would it be better
> > to migrate away from looking up a relation oid in one function and opening the
> > relation in another function, instead passing either a Relation or a RangeVar?
> > You did substitute passing a Relation in a couple of places.

[reasons this is a can of worms]
> At any rate, I'm not inventing this problem; there are plenty of
> existing instances where this same phenomenon occurs. At least I'm
> documenting the ones I'm adding. There's probably room for further
> improvement and restructuring of this code, but right at the moment I
> feel like the reasonable alternatives are (a) to pass a lock level
> that matches what will ultimately be taken later on or (b) pass
> NoLock. I'm voting for the former.

Works for me.