Re: Event Triggers reduced, v1

On Fri, Jul 6, 2012 at 3:29 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Here is an incremental documentation patch which I hope you will like.
>
> Definitely, it's better this way. I'm not thrilled with separating it
> into its own top level chapter, but I can see how it makes sense indeed.

Oh, really? I thought that was a huge readability improvement. There
are some things that are the same between triggers and event triggers,
but there's an awful lotta stuff that is completely different.

> This part is strange though:
>
> + A trigger definition can also specify a <literal>WHEN</literal>
> + condition so that, for example, a <literal>command_start</literal>
> + tag can be fired only for particular commands which the user wishes
> + to intercept. A common use of such triggers is to restrict the range of
> + DDL operations which users may perform.
>
> I don't think of that as firing a command tag, so it's hard for me to
> parse that sentence.

Oh, that should say "a command_start trigger" rather than "a
command_start tag". Good catch.

>> This took the last several hours, so I haven't looked at your latest
>> code changes yet. However, in the course of editing the
>> documentation, it occurred to me that we seem to be fairly arbitrarily
>> excluding a large number of commands from the event trigger mechanism.
>
> As many as that? I'm surprised about the quantity. Yes I did not add all
> and any command we have, on purpose, and I agree that the new turn of
> things allow us to add a new set.

I admit I didn't count them up. :-) Maybe there aren't that many.

I think we might want to extend the support matrix to include every
command that appears in sql-commands.html and have either an X if it's
supported or blank if it's not. That would make it easier to judge
how many commands are not supported, not just for us but for users who
may be trying to answer the same questions we are.

> I would think that NOTIFY is on a fast track not to be disturbed by
> calling into used defined code, and that would explain why we don't
> support event triggers here.

If the DBA is allowed to restrict CREATE FUNCTION, why not NOTIFY? I
guess I don't see why that one's deserving of special treatment.

Mind you, if I ran the world, this would probably be broken up
differently: I'd have ddl_command_start covering all the
CREATE/ALTER/DROP commands and nothing else; and separate firing
points for anything else I wanted to support. It's not too late to
make that change, hint, hint. But if we're not gonna do that then I
think that we'd better try to cast the net as broadly as reasonably
possible. It seems to me that our excuse for not including things
like UPDATE and DELETE is a bit thin; surely there are people who
would like a sort of universal trigger that applies to every relation
in the system. Of course there are recursion problems there that need
to be thought long hard about, and no I don't really want to go there
right now, but I'll bet you a nickle that someone is going to ask why
it doesn't work that way.

Another advantage to recasting this as ddl_command_start is that we
quite easily pass the operation (CREATE, ALTER, DROP) and the named
object type (TABLE, FUNCTION, CAST) as separate arguments. I think
that would be a usability improvement, since it would then be dead
easy to write an event trigger that prohibits DROP (and only DROP) of
any sort. Of course it's not that hard to do it right now, but you
have to parse the command tag. It would likely simplify the code for
mapping between node and command tags, too.

One other thought: if we're NOT going to do what I suggested above,
then how about renaming TG_WHEN to TG_EVENT? Seems like that would
fit better.

Also: now that the E_WhatEver constants don't get stored on disk, I
don't think they should live in pg_event_trigger.h any more; can we
move them to someplace more appropriate? Possibly make them private
to event_trigger.c? And, on a related note, I don't think it's a good
idea to use E_ as a prefix for both the event types and the command
tags. It's too short, and hard to grep for, and we don't want the
same one for both, I think. How above things like EVT_CommandStart
for the events and ECT_CreateAggregate for the command tags?

>> I had an interesting experience while testing this patch. I
>> accidentally redefined my event trigger function to something which
>> errored out. That of course precluded me from using CREATE OR REPLACE
>> FUNCTION to fix it. This makes me feel rather glad that we decided to
>> exclude CREATE/ALTER/DROP EVENT TRIGGER from the event trigger
>> mechanism, else recovery would have had to involve system catalog
>> hackery.
>
> Yeah, we have some places were it's not very hard to shoot oneself in
> the foot, here the resulting hole is a little too big and offers no real
> benefits. Event triggers on create|alter|drop event triggers, really?

Indeed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company