Re: Command Triggers patch v18

On Tue, Mar 27, 2012 at 4:27 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)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