Re: Event Triggers reduced, v1

On Fri, Jun 15, 2012 at 4:27 PM, Dimitri Fontaine
<dfontaine(at)hi-media(dot)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