Re: Command Triggers patch v18

On Thu, Mar 29, 2012 at 8:30 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)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