Re: Event Triggers reduced, v1

On Wed, Jun 20, 2012 at 4:36 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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
>> event types will be more like "during" rather than "before" or
>> "after", and for those that do have a notion of before and after, we
>> can have two different event names and include the word "before" or
>> "after" there.  I am otherwise satisfied with the schema you've
>> chosen.
>
> It's not before/after anymore, but rather addon/replace if you will. I
> kept the INSTEAD OF keyword for the replace semantics, that you've been
> asking me to keep IIRC, with security policy plugins as a use case.
>
> Now we can of course keep those semantics and embed them in the event
> name we provide users, I though that maybe a documentation matrix of
> which event support which "mode" would be cleaner to document. We might
> as well find a clean way to implement both modes for most of the
> commands, I don't know yet.
>
> So, are you sure you want to embed that part of the event trigger
> semantics in the event name itself?

Yeah, pretty sure. I think that for regular triggers, BEFORE, AFTER,
and INSTEAD-OF are the firing-point specification. But even triggers
will have more than three firing points, probably eventually quite a
lot more. So we need something more flexible. But we don't need that
more flexible thing AND ALSO the before/after/instead-of
specification, which I think in most cases won't be meaningful anyway.
It happens to be somewhat sensible for this initial firing point, but
I think for most of them there will be just one place, and in many
cases it will be neither before, nor after, nor instead-of.

>> 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
>
> I've been trying to do that yes, as you can see with event_name and
> event_trigger_variable rules. I've been re-using as much existing
> keywords as I could because I believe that's not causing any measurable
> bloat, I'll kindly reconsider if necessary, even if sadly.

The issue is that the size of the parser tables grow with the square
of the number of states. This will introduce lots of new states that
we don't really need; and every new kind of event trigger that we want
to add will introduce more.

>> 3. The event trigger cache seems to be a few bricks shy of a load.
>
> I wouldn't be that surprised, mind you. I didn't have nearly as much
> time I wanted to working on that project.
>
>> First, event_trigger_cache_is_stalled is mis-named; I think you mean
>> "stale", not "stalled".  Second, instead of setting that flag and then
>
> Stale. Right. Edited.
>
>> rebuilding the cache when you see the flag set, how about just blowing
>> away the cache contents whenever you would have set the flag?  That
>
> I've been doing that at first, but that meant several full rebuilds in a
> row in the regression tests, which are adding new event triggers then
> using them. I though lazily maintaining the cache would be better.

Well, AFAICS, you're still doing full rebuilds whenever something
changes; you're just keeping the (useless, dead) cache around until
you decide to rebuild it. Might as well free the memory once you know
that the next access will rebuild it anyway, and for a bonus it saves
you a flag.

> I'm not that fond of psql commands, but I don't think it's going to fly
> not to have one for event triggers. I could buy \dy.

Yeah, I think people are going to want to have one. I really despise
the \d<whatever> syntax, but it's not 100% clear what a better one
would look like.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company