On Thu, Mar 29, 2012 at 8:30 AM, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: > I'll go make that happen, and still need input here. We first want to > have command triggers on specific commands or ANY command, and we want > to implement 3 places from where to fire them. > > Here's a new syntax proposal to cope with that: > > create command trigger before COMMAND_STEP of alter table > execute procedure snitch(); > > - before the process utility switch, with only command tag and parse > tree > > create command trigger foo before start of alter table > execute procedure snitch(); > > - before running the command, after having done basic error checks, > security checks, name lookups and locking, with all information > > create command trigger before execute of alter table > execute procedure snitch(); > > - after having run the command > > create command trigger foo before end of alter table > execute procedure snitch();
One thought is that it might be better to say AT or ON or WHEN rather than BEFORE, since BEFORE END is just a little strange; and also because a future hook point might be something like "permissions-checking", and we really want it to be AT permissions-checking time, not BEFORE permissions-checking time. If I got to pick, I'd pick this syntax: CREATE EVENT TRIGGER name ON event_name (event_subtype_name [, ...]) EXECUTE PROCEDURE function_name(args); Then you could eventually imagine: CREATE EVENT TRIGGER prohibit_all_ddl ON ddl_command_start EXECUTE PROCEDURE throw_an_error(); CREATE EVENT TRIGGER prohibit_some_ddl ON ddl_command_start (alter_table) EXECUTE PROCEDURE throw_an_error(); CREATE EVENT TRIGGER replicate_table_drops ON sql_drop (table) EXECUTE PROCEDURE record_table_drop(); CREATE EVENT TRIGGER allow_whitelisted_extensions ON permissions_check (create_extension) EXECUTE PROCEDURE make_heroku_happy(); CREATE EVENT TRIGGER replicate_at_subcommands ON subcommand_start (alter_table) EXECUTE PROCEDURE record_alter_table_subcommand(); Piece by piece: - I prefer EVENT to COMMAND, because the start or end or any other point during the execution of a command can be viewed as an event; however, it is pretty clear that not everything that can be viewed as an event upon which we might like triggers to fire can be viewed as a command, even if we have a somewhat well-defined notion of subcommands. I continue to think that there is no sense in which creating a sequence to back a serial column or dropping an object due to DROP .. CASCADE is a subcommand; it is, however, pretty clearly an event. Anything we want can be an event. I think if we don't get this right in the first version we're going to be stuck with a misnomer forever. :-( - It is pretty clear that for triggers that are supposed to fire at the start or end of a command, it's useful to be able to specify which commands it fires for. However, there must be other types of events (as in my postulated sql_drop case above) where the name of the command is not relevant - people aren't going to find all the objects that got dropped as a result of a toplevel drop table command; they're going to want to find all the tables that got dropped regardless of which incantation did it. Also keep in mind that you can do things like use ALTER TABLE to rename a view (and there are other cases of that sort of confusion with domains vs. types, aggregates vs. functions, etc), so being able to filter based on object type seems clearly better in a bunch of real-world cases. And, even if you don't believe that specific example, a general syntax leaves us with freedom to maneuver if we discover other needs in the future. Writing alter_table etc. as tokens rather than as ALTER TABLE also has the further advantages of (1) greatly decreasing the parser footprint of this feature and (2) relieving everyone who adds commands in the future of the need to also change the command-trigger grammar. - I don't think that triggers at what Dimitri is calling "before execute" are a good idea at all for the first version of this feature, because there is a lot of stuff that might break and examining that should really be a separate project from getting the basic infrastructure in place. However, I also don't like the name. Unlike queries, DDL commands don't have clearly separate planning and execution phases, so saying that something happens before execution isn't really very clear. As previously discussed, the hook points in the latest patch are not all entirely consistent between one command and the next, not only in the larger ways I've already pointed out but also in some smaller ways. Different commands do different kinds of integrity checks and the firing points are not always in exactly the same place in that sequence. Of course, not all the commands do all the checks in the same order, so some possibly-not-trivial refactoring is probably required to get that consistent. Moreover, it doesn't seem out of reach to think that in the future we might draw a more clear line again between pre-execution and execution proper, to solve problems like the tendency of the current code to look up the same name more than once and pray that it gets the same OID back every time. So I think that when we get to the point of sticking hooks in at this location, we should call them something much more specific than before-execute hooks. I'm not sure exactly what. Maybe something like after_ddl_name_lookup or something like that. The DDL name lookup might move slightly earlier or later as a result of refactoring, but it can't disappear and it can't cease to involve locking and permissions checking. The set of things that you can reasonably do there (additional security checks, auditing) can't really change much either. -- 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
