On Fri, Jul 6, 2012 at 3:29 PM, Dimitri Fontaine <dimi...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
