On Fri, Jun 15, 2012 at 4:27 PM, Dimitri Fontaine <dfonta...@hi-media.com> wrote: > Allow me to open the new season of the DML trigger series, named > pg_event_trigger. This first episode is all about setting up the drama, > so that next ones make perfect sense.
Comments: 1. I still think we ought to get rid of the notion of BEFORE or AFTER (i.e. pg_event_trigger.evttype) and just make that detail part of the event name (e.g. pg_event_trigger.evtevent). Many easily forseeable event types will be more like "during" rather than "before" or "after", and for those that do have a notion of before and after, we can have two different event names and include the word "before" or "after" there. I am otherwise satisfied with the schema you've chosen. 2. I think it's important to be able to add new types of event triggers without creating excessive parser bloat. I think it's important to use some kind of generic syntax here which will be able to apply to all types of triggers we may want to add, now or in the future. The easiest way to do that is to use literal syntax for the list of command tags, rather than writing them out as key words: e.g. 'ALTER TABLE' rather than ALTER TABLE. It's not quite as pretty, but the savings in parser bloat and future code change seem well worth it. Or, alternatively, we could use identifier style, e.g. alter_table, as I previously suggested. 3. The event trigger cache seems to be a few bricks shy of a load. First, event_trigger_cache_is_stalled is mis-named; I think you mean "stale", not "stalled". Second, instead of setting that flag and then rebuilding the cache when you see the flag set, how about just blowing away the cache contents whenever you would have set the flag? That seems a whole lot simpler and cleaner, and removes the need for a force_rebuild flag on BuildEventTriggerCache(). Third, ISTM that this isn't going to work correctly if backend A performs an event after backend B has built its cache. To fix this, I think you need to rip out all the places where you force a rebuild locally and instead use something like CacheRegisterSyscacheCallback() to blow away the cache whenever something changes; you might find it helpful to look at attoptcache.c. 4. The documentation doesn't build. openjade:reference.sgml:44:4:W: cannot generate system identifier for general entity "alterEventTrigger" openjade:reference.sgml:44:4:E: general entity "alterEventTrigger" not defined and no default entity openjade:reference.sgml:44:21:E: reference to entity "alterEventTrigger" for which no system identifier could be generated openjade:reference.sgml:44:3: entity was defined here openjade:reference.sgml:86:4:W: cannot generate system identifier for general entity "createEventTrigger" openjade:reference.sgml:86:4:E: general entity "createEventTrigger" not defined and no default entity openjade:reference.sgml:86:22:E: reference to entity "createEventTrigger" for which no system identifier could be generated openjade:reference.sgml:86:3: entity was defined here openjade:reference.sgml:125:4:W: cannot generate system identifier for general entity "dropEventTrigger" openjade:reference.sgml:125:4:E: general entity "dropEventTrigger" not defined and no default entity openjade:reference.sgml:125:20:E: reference to entity "dropEventTrigger" for which no system identifier could be generated openjade:reference.sgml:125:3: entity was defined here openjade:catalogs.sgml:1868:35:E: character "_" is not allowed in the value of attribute "ZONE" openjade:catalogs.sgml:1868:19:X: reference to non-existent ID "CATALOG-PG-EVENT_TRIGGER" openjade:trigger.sgml:43:47:X: reference to non-existent ID "SQL-CREATECOMMANDTRIGGER" 5. In terms of a psql command, I think that \dev is both not very mnemonic and, as you mentioned in the comment, possibly confusable with SQL/MED. If we're going to have a \d command for this, I suggest something like \dy, which is not very mnemonic either but at least seems unlikely to be confused with anything else. Other things that are unused include \dh, \dj, \dk, \dq, \dw, and \dz, if any of those grab you, or a somewhat broader range of things (but still nothing very memorable) if we include capital letters. Or we could branch out into punctuation, like \d& -- or things that don't begin with the letter d, but that doesn't seem like a particularly good idea. 6. In objectaddress.c, I think that get_object_address_event_trigger should be eliminated in favor of an additional case in get_object_address_unqualified. 7. There are many references to command triggers that still need to be cleaned up. trigger.sgml still makes reference to the name command triggers. plpgsql.sgml also contains vestiges of the command trigger notation, and references to some TG_* variables that I don't think exist in this version of the patch. event_trigger.c is identified (twice) as cmdtrigger.c in the file header comment. The header comment for InsertEventTriggerTuple makes reference to pg_cmdtrigger, as does a line comment in RemoveEventTriggerById. The regression output mentions cmdtrigger in a few places as well. In the documentation, event triggers are mentioned as having return type command_trigger, but it's now called event_trigger. 8. The re-addition of node tags like T_RemoveFuncStmt seems to be a merging error. Changing \dc so that it rejects \dcrap appears to be a leftover from when the command was \dcT. In one place in the docs you have 'avent' for 'event'. In event_trigger.c, you have #ifdef UNDEFINED; project standard is #ifdef NOT_USED (or else you can remove the code). 9. The regression tests seem to now be testing some features that don't exist any more, and might need some rethinking to make what they do match the scope of this patch. 10. I suggest separating out the support for other PLs (Python, Tcl) and submitting that as a later patch, since I'm unqualified to commit it (and I'm hoping to get the rest of this committed). The PL/TCL stuff also contains residual references to the command-trigger notation which should be cleaned up before resubmitting. There's probably more, but I'm all reviewed out for right now. Hopefully that's enough to get you started. I think this is heading in a good direction, even though there's still a good bit of work left to do. -- 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
