Hi,
Here's an early revision 2 of the patch, not yet ready for commit, so
including the PL stuff still. What's missing is mainly a cache reference
leak to fix at DROP EVENT TRIGGER, but I failed to spot where it comes
from.
As I fixed about all the other comments I though I might as well send in
the new version of the patch to get to another round of review.
Robert Haas <robertmhaas@gmail.com> writes:
> 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
So, agreed on before/after, not on INSTEAD OF. No change in the patch,
still discussing that point.
> 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
Fixed in the attached, I believe.
> 3. The event trigger cache seems to be a few bricks shy of a load.
Fixed in the attached, including cache invalidation registered as a
system cache callback.
> 4. The documentation doesn't build.
Fixed, I mainly managed to forget adding the new files.
> 5. In terms of a psql command, I think that \dev is both not very
Switched to \dy and cleaned up.
> 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.
Fixed in the attached.
> 7. There are many references to command triggers that still need to be
> cleaned up.
All fixed.
> 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).
All fixed.
> 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.
Actually those tests helped me spot some strange things when cleaning up
the cache key, and only some commands would fail. So I'm in favor of
keeping it all for now.
> 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.
That's for next turn.
> 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.
So, let's see about that remaining bit of work :)
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support