Re: Event Triggers reduced, v1

Hi,

Please find attached v6 of the patch, fixing all your comments here.
Sorry about missing some obvious things in the making of this patch.

Given that I had to merge master again, I can't easily produce an
incremental patch on top of last version, so you only have the full
patch attached.

Robert Haas <robertmhaas@gmail.com> writes:
> - 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.

Done. Removed now useless exposed functions.

> - When you removed format_type_be_without_namespace, you didn't remove
> either the function prototype or the related changes in format_type.c.

Fixed.

> - 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.

I'm failing to reproduce it here, in -O0. […Trying with the -O2
default…] I got the error in -O2, fixed in the attached.

> - The definitions of EVTG_FIRED_* are still present in
> pg_event_trigger.h, even though they're longer used anywhere.

Fixed.

> - 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.

Fixed. I only found a single such vestige in plperl.sgml though.

> 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.)

Damn, I had it on my TODO and Álvaro hinted me already, and I kept
forgetting about it nonetheless. Fixed now.

> - 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?).

Agreed, done that way (using a NameData fixed width field).

> - 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.

Fixed.

> - 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.

D'oh. Sure. Done.

> - Why did you create a separate file pg_event_trigger_fn.h instead of
> just including that single prototype in pg_event_trigger.h?

To mimic some existing files, fixed.

> - 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.

It's now used in more places (dependencies, alter owner), so thinking
that it's pulling its weight now with 3 call sites I've left it alone.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support