On Thu, Jan 17, 2013 at 12:06 PM, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: > Ok. Will prepare a non controversial patch for ddl_command_end.
Thanks. I will make a forceful effort to review that in a timely fashion when it's posted. >> I think this is a bad idea, not only because, as I said before, it >> exposes internal implementation details, but also because it's >> probably not safe. As Noah (I believe, or maybe Tom), observed in a >> previous post, we've gone to a lot of work to make sure that nothing >> you do inside a trigger can crash the server, corrupt the catalogs, >> trigger elog() messages, etc. That applies in spades to DDL. > > I naively though that doing CommandCounterIncrement() before running the > event trigger would go a long enough way towards making the operation > safe when the current code is calling back into ProcessUtility(). It's something, but I'd be shocked if it covers all the bases. Let me try to give a concrete example of how I think another firing point could be made to work along the lines I'm suggesting. I'm not going to use DROP as an example because I think upon reflection that will be a particularly challenging case to make work correctly. Instead, let me use the example of ALTER. Goal: Every time an ALTER command is used on object *that actually exists*, we will call some user-defined function and pass the object type, the OID of the object, and some details about what sort of alteration the user has requested. Where shall we put the code to do this? Clearly, ProcessUtility is too early, because at that point we have not done the name lookup, locked the object, or checked permissions. So the OID of the target object is not and cannot be known at that point. We cannot do an extra name lookup at that point without taking a lock, because the answer might change, or, due to non-MVCC catalog access, the lookup might fail to find an object altogether. And we can't take a lock without checking permissions first. And the permissions-checking logic can't be done inside ProcessUtility(), because it's object-type specific and we don't want to duplicate it. So, instead, what we need to do is go into each function that implements ALTER, and modify it so that after it does the dance where we check permissions, take locks, and look up names, we call the trigger function. That's a bit of a nuisance, because we probably have a bunch of call sites to go fix, but in theory it should be doable. The problem of course, is that it's not intrinsically safe to call user-defined code there. If it drops the object and creates a new one, say, hilarity will probably ensue. But that should be a fixable problem. The only things we've done at this point are check permissions, lock, and look up names. I think we can assume that if the event trigger changes permissions, it's safe for the event trigger to ignore that. The trigger cannot have released the lock, so we're OK there. The only thing it can really have done that's problematic is change the results of the name lookup, say by dropping or renaming the object. But that's quite simple to protect against: after firing the trigger, we do a CommandCounterIncrement() and repeat the name lookup, and if we get a different answer, then we bail out with an error and tell the user they're getting far too cute. Then we're safe. Now, there is a further problem: all of the information about the ALTER statement that we want to provide to the trigger may not be available at that point. Simple cases like ALTER .. RENAME won't require anything more than that, but things like ALTER TABLE .. ADD FOREIGN KEY get tricky, because while at this point we have a solid handle on the identity of the relation to which we're adding the constraint, we do NOT yet have knowledge of or a lock on the TARGET relation, whose OID could still change on us up to much later in the computation. To get reliable information about *that*, we'll need to refactor the sequence of events inside ALTER TABLE. Hopefully you can see what I'm driving toward here. In a design like this, we can pretty much prove - after returning from the event trigger - that we're in a state no different from what would have been created by executing a series of commands - lock table, then SELECT event_trigger_func(), then the actual ALTER in sequence. Maybe there's a flaw in that analysis - that's what patch review is for - but it sounds pretty darn tight to me. I could write more, but I have to take the kids to dance class, and this email may be too long already. Does that help at all to clarify the lines along which I am thinking? Note that the above is clearly not ddl_command_start but something else, and the different firing point and additional safety cross-checks are what enable us to pass additional information to the trigger without fear of breaking either the trigger or the world. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
