Re: Make relation_openrv atomic wrt DDL

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

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

>> +             /*
>> +              * 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).

> ... also making it cleaner to preserve this optimization.

That optimization is now gone anyway.

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

Well, I've got these:

- ReindexTable() must match reindex_relation()
- ExecRenameStmt() must match RenameRelation()
- DropTrigger() must match RemoveTriggerById()
- DefineRule() must match DefineQueryRewrite()
- RemoveRewriteRule() must match RemoveRewriteRuleById()

(Whoever came up with these names was clearly a genius...)

RemoveTriggerById() and RemoveRewriteRuleById() are part of the
drop-statement machinery - they can be called either because that
rule/trigger itself gets dropped, or because some other object gets
dropped and it cascades to the rule/trigger. I'm pretty sure I don't
want to go start mucking with that machinery.

On the other hand, RenameRelation() is only called in one place other
than ExecRenameStmt(), and it looks to me like the other caller could
just as well use RenameRelationInternal(). If we change that, then we
can redefine RenameRelation() to take a RangeVar instead of an Oid and
push the rest of the logic from ExecRenameStmt() into it. That would
probably be better all around.

The situation with DefineRule and DefineQueryRewrite is a bit more
nuanced. As you suggest, those could probably be merged into one
function taking both an Oid and a RangeVar. If the Oid is not
InvalidOid, we do heap_open(); else, we do heap_openrv(). That's
slightly ugly, but there are only two callers, so maybe it's not so
bad.

Offhand, I don't see any good way to rearrange reindex_relation()
along those lines. ReindexTable() wants to check permissions, not
just open the relation. And passing a Relation to reindex_relation()
rather than an Oid or RangeVar is no good either; you still end up
with multiple people needing to know what lock level to use.

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.

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