On Thu, Jul 7, 2011 at 3:54 PM, Noah Misch <n...@2ndquadrant.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
atomic-rangevargetrelid-v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
