On Tue, Mar 27, 2012 at 4:27 AM, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: > In the first versions of the patch I did try to have a single point > where to integrate the feature and that didn't work out. If you want to > publish information such as object id, name and schema you need to have > specialized code spread out everywhere. > [...] > > That's meant to be a feature of the command trigger system, that's been > asked about by a lot of people. Having triggers fire for sub commands is > possibly The Right Thing™, or at least the most popular one. > [...] > > I will rework LOAD, and ALTER TABLE too, which is more work as you can > imagine, because now we have to insinuate the command trigger calling > into each branch of what ALTER TABLE is able to implement. > [...] > > From last year's cluster hacker meeting then several mails on this list, > it appears that we don't want to do it the way you propose, which indeed > would be simpler to implement. > [...] > > I think that's a feature. You skip calling the commands trigger when you > know you won't run the command at all.
I am coming more and more strongly to the conclusion that we're going in the wrong direction here. It seems to me that you're spending an enormous amount of energy implementing something that goes by the name COMMAND TRIGGER when what you really want is an EVENT TRIGGER. Apparently, you don't want a callback every time someone types CREATE TABLE: you want a callback every time a table gets created. You don't want a callback every time someone types DROP FUNCTION; you want a callback every time a function gets dropped. If the goal here is to do replication, you'd more than likely even want such callbacks to occur when the function is dropped indirectly via CASCADE. In the interest of getting event triggers, you're arguing that we should contort the definition of the work "command" to include sub-commands, but not necessarily commands that turn out to be a no-op, and maybe things that are sort of like what commands do even though nobody actually ever executed a command by that name. I just don't think that's a good idea. We either have triggers on commands, and they execute once per command, period; or we have triggers on events and they execute every time that event happens. As it turns out, two people have already put quite a bit of work into designing and implementing what amounts to an event trigger system for PostgreSQL: me, and KaiGai Kohei. It's called the ObjectAccessHook mechanism, and it fires every time we create or drop an object. It passes the type of object created or dropped, the OID of the object, and the column number also in the case of a column. The major drawback of this mechanism is that you have to write the code you want to execute in C, and you can't arrange for it to be executed via a DDL command; instead, you have to set a global variable from your shared library's _PG_init() function. However, I don't think that would be very hard to fix. We could simply replaces the InvokeObjectAccessHook() macro with a function that calls a list of triggers pulled from the catalog. I think there are a couple of advantages of this approach over what you've got right now. First, there are a bunch of tested hook points that are already committed. They have well-defined semantics that are easy to understand: every time we create or drop an object, you get a callback with these arguments. Second, KaiGai Kohei is interested in adding more hook points in the future to service sepgsql. If the command triggers code and the sepgsql code both use the same set of hook points then (1) we won't clutter the code with multiple sets of hook points and (2) any features that get added for purposes of command triggers will also benefit sepgsql, and visca versa. Since the two of you are trying to solve very similar problems, it would be nice if both of you were pulling in the same direction. Third, and most importantly, it seems to match the semantics you want, which is a callback every time something *happens* rather than a callback every time someone *types something*. Specifically, I propose the following plan: - Rename COMMAND TRIGGER to EVENT TRIGGER. Rewrite the documentation to say that we're going to fire triggers every time an *event* happens. Rewrite the code to put the firing mechanism inside InvokeObjectAccessHook, which will become a function rather than a macro. - Change the list of supported trigger types to match what the ObjectAccessHook mechanism understands, which means, at present, post-create and drop. It might even make sense to forget about having separate hooks for every time of object that can be created or dropped and instead just let people say CREATE EVENT TRIGGER name ON { CREATE | DROP } EXECUTE PROCEDURE function_name ( arguments ). - Once that's done, start adding object-access-hook invocations (and thus, the corresponding command trigger functionality) to whatever other operations you'd like to have support it. I realize this represents a radical design change from what you have right now, but what you have right now is messy and ill-defined and I don't think you can easily fix it. You're exposing great gobs of implementation details which means that, inevitably, every time anyone wants to refactor some code, the semantics of command triggers are going to change, or else the developer will have to go to great lengths to ensure that they don't. I don't think either of those things is going to make anyone very happy. -- 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