Re: Event Triggers reduced, v1

On Tue, Jul 10, 2012 at 10:38 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
>> 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
>
> Let's see about doing that. I guess we would have ddl_command_start and
> command_start, and I would think that the later is the most generic. So
> we would certainly want DDL commands to run first the command_start
> event triggers then the ddl_command_start event triggers, whereas a
> NOTIFY would only run command_start triggers, and a GRANT command would
> run maybe command_start then dcl_command_start triggers?
>
> If that's where we're going to, we can commit as-is and expand later.

That's not quite what I was thinking. I actually can't imagine any
situation where you want an event trigger that gets fired on EVERY
command for which we can support command_start. If you're trying to
prevent or replicate DDL, that's too much. If you're trying to do
logging or auditing, it's not enough, since there will still be
commands that aren't supported, and it'll be grossly inefficient to
boot. You really want something like log_min_duration_statement=0 for
those cases. So it seems to me that the use case for a command_start
trigger, conceived in the broadest possible way so that every single
command we can support is included, is razor-thin.

So my proposal for the present patch would be:

1. Rename command_start to ddl_command_start.
2. Remove support for everything other than CREATE, ALTER, and DROP.
3. Pass the operation and the SQL object type as separate magic variables.

Then we can add dcl_command_start, etc. in follow-on patches.

>> 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.
>
> The current reason why we only support 149 SQL commands and variations
> is because we want a patch that's easy enough to review and agree on. So
> I think we will in the future be able to add new firing point at places
> where maybe some discussion is needed.

Agreed.

> Such places, in my mind, include the NOTIFY mechanism, DCLs, and global
> objects such as databases and tablespaces and roles. I'd be happy to see
> event triggers embrace support for those. Maybe in v2 though?

Yep, sure. Note that the proposal above constrains the list of
commands we support in v1 in a very principled way: CREATE, ALTER,
DROP. Everything else can be added later under a different (but
similarly situated) firing point name. If we stick with command_start
then I think we're going to be forever justifying our decisions as to
what got included or excluded; which might be worth it if it seemed
likely that there'd be much use for such a command trigger, but it
doesn't (to me, anyway).

>> 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's a good idea. I don't think we should replace the current tag
> support with that though, because some commands are harder to stow into
> the operation and type model (in supported commands, mainly LOAD).

I'm imagining that ddl_command_start triggers would get the
information this way, but LOAD might be covered by something like
admin_command_start that just gets the command tag.

> So I've included partial support for that in the attached patch, in the
> simplest way possible, just so that we can see where it leads in term of
> using the feature. The next step here is to actually go in each branch
> of the process utility switch and manually decorate the command context
> with the current operation and objecttype when relevant.
>
[...]
> Done in the attached. Filling that array was… an interesting use case
> for Emacs Keyboard Macros spanning 3 different buffers, maintaining it
> should be easy enough now.

Yep, looks better. It looks like you've got
EventTriggerCommandTagsEntry mapping the command tag to an ETC_*
constant; I think the need for that hash goes away entirely if you
just pass this information down from the ProcessUtility() switch. At
any rate having NameData involved seems like it's probably not too
good an idea; if for some reason we need to keep that hash, use a
NUL-terminated string and initialize the hash table with string_hash
instead of tag_hash. That'll be simpler and also allows the length of
the buffer to vary independently of NAMEDATALEN, which can only be to
the good.

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