On Tue, Dec 06, 2011 at 04:02:31PM -0500, Robert Haas wrote: > On Mon, Dec 5, 2011 at 2:09 AM, Noah Misch <n...@leadboat.com> wrote: > >> ? ? ? ? ? ? ? AcceptInvalidationMessages(); > > > > The above call can go away, now. > > Doesn't that still protect us against namespace-shadowing issues? > RangeVarGetRelid doesn't actually AcceptInvalidationMessages() at the > top.
It narrows the window for race conditions of that genesis, but isn't doing so an anti-feature? Even if not, doing that _only_ in RemoveRelations() is odd. FWIW, this is fairly independent of your latest patches -- I just happened to notice it due to the incidental quotation. > Attached please find a patch with some more fixes on this same general > theme. This one tackles renaming of relations, columns, and triggers; > and changing the schema of relations. In these cases, the current > code does a permissions check before locking the table (which is good) > and uses RangeVarGetRelid() to guard against "cache lookup failure" > errors caused by concurrent DDL (also good). However, if the referent > of the name changes during the lock wait, we don't recheck > permissions; we just allow the rename or schema change on the basis > that the user had permission to do it to the relation that formerly > had that name. While this is pretty minor as security concerns go, it > seems best to clean it up, so this patch does that. I'd suggest limiting callback functions to checks that benefit greatly from preceding the lock acquisition. In these cases, that's only the target object ownership check; all other checks can wait for the lock. Writing correct callback code is relatively tricky; we have no lock, so the available catalog entries can change arbitrarily often as the function operates. It's worth the trouble of navigating that hazard to keep the mob from freely locking all objects. However, if the owner of "some_table" writes "ALTER VIEW some_table RENAME TO foo", I see no commensurate benefit from reporting the relkind mismatch before locking the relation. This would also permit sharing a callback among almost all the call sites you've improved so far. Offhand, I think only ReindexIndex() would retain a bespoke callback. This patch removes two of the three callers of CheckRelationOwnership(), copying some of its logic to two other sites. I'd suggest fixing the third caller (standard_ProcessUtility() for CREATE INDEX) in this same patch. That way, we can either delete the function now or adjust it to permit continued sharing of that code under the new regime. I like how this patch reduces the arbitrary division of authorization checks between alter.c and tablecmds.c. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers