Re: Event Triggers reduced, v1 - Mailing list pgsql-hackers

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