On Wed, Jul 4, 2012 at 1:56 PM, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: > Dimitri Fontaine <dimi...@2ndquadrant.fr> writes: >> Robert Haas <robertmh...@gmail.com> writes: >>> No, I'm not asking you to add any more columns right now (in fact, >>> please do not). But the type of the existing column should change to >>> text[]. >> >> Ok, done in the attached. We now read text from either the user input in >> the grammar or from the catalogs (in a 1-D array there), and convert to >> our enum structure for internal code use. > > In the previous one I missed adapting pg_dump and psql, here's the > updated set of patches. Sorry about that. This unexpected electrical > shut down in the middle of the day was not cool. Nor was the second one.
Thanks. There are some review comments from previous rounds that don't seem to have been fully incorporated to this version: - You only changed the definition of pg_event_triggers.evttags, not the documentation. I don't think we need pg_evttag_to_string aka pg_event_trigger_command_to_string any more either. - When you removed format_type_be_without_namespace, you didn't remove either the function prototype or the related changes in format_type.c. - I'm pretty sure that a previous review mentioned the compiler warning in evtcache.c, which seems not to be fixed. It doesn't look harmless, either. - The definitions of EVTG_FIRED_* are still present in pg_event_trigger.h, even though they're longer used anywhere. - The comments in parsenodes.h still refer to command triggers rather than event triggers. event_trigger.h contains a reference to CommandTriggerData that should say EventTriggerData. plperl.sgml has vestiges of the old terminology as well. In terms of the schema itself, I think we are almost there, but: - I noticed while playing around with this that pg_dump emits a completely empty owner field when dumping an event trigger. At first I thought that was just an oversight, but then I realized there's a deeper reason for it: pg_event_trigger doesn't have an owner field. I think it should. The only other objects in the system that don't have owners are text search parsers and text search templates (and casts, sort of). It might seem redundant to have an owner even when event-triggers are superuser-only, but we might want to try to relax that restriction later. Note that foreign data wrappers, which are also superuser-create-only, do have an owner. (Note that if we give event triggers an owner, then we also need ALTER .. OWNER TO support for them.) - It seems pretty clear at this point that the cache you've implemented in src/backend/utils/cache/evtcache.c is going to do all the heavy lifting of converting the stored catalog representation to a form that is suitable for quick access. Given that, I wonder whether we should go whole hog and change evtevent to a text field. This would simplify things for pg_dump and we could get rid of pg_evtevent_to_string, at a cost of doing marginally more work when we rebuild the event cache (but who really cares, given that you're reading the entire table every time you rebuild the cache anyway?). Thoughts? Some other minor comments: - In the \dy output, command_start is referred to as the "condition", but the documentation calls it an "event" and the grammar calls it "event_name". "event" or "event_name" seems better, so I think this is just a matter of changing psql to match. - AlterEventTrigStmt defines tgenbled as char *; I think it should just be char. In gram.y, you can declare the enable_trigger production as <chr> (or <ival>?) rather than <str>. At any rate pstrdup(stmt->tgenabled)[0] doesn't look right; you don't need to copy a string to fetch the first byte. - Why did you create a separate file pg_event_trigger_fn.h instead of just including that single prototype in pg_event_trigger.h? - The EVENTTRIGGEROID syscache is used only in RemoveEventTriggerById. I don't think that's a sufficient justification for eating up more memory for another system cache. I think you should just remove that cache and recode RemoveEventTriggerById to find the correct tuple via an index scan. See RemoveEventTriggerById for an example. -- 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
