Re: Event Triggers reduced, v1

On Wed, Jul 4, 2012 at 1:56 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
>> Robert Haas <robertmhaas(at)gmail(dot)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