Re: Event Triggers reduced, v1

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

I agree with using the event name as a the specification for the firing
point, and that we should prefer documenting the ordering of those
rather than offering a fuzzy idea of BEFORE and AFTER steps in there.
The AFTER step is better expressed as BEFORE the next one.

Now, I still think there's an important discrepancy between adding a new
behaviour that adds-up to whatever the backend currently implements and
providing a replacement behaviour with a user defined function that gets
called instead of the backend code.

And I still don't think that the event name should be carrying alone
that semantic discrepancy. Now, I also want the patch to get in, so I
won't insist very much if I'm alone in that position. Anyone else
interested enough to chime in?

The user visible difference would be between those variants:
 create event trigger foo at 'before_security_check' ... create event trigger foo at 'replace_security_check' ...
 create event trigger foo before 'security_check' ... create event trigger foo instead of 'security_check' ...

Note that in this version the INSTEAD OF variant is not supported, we
only intend to offer it in some very narrow cases, or at least that is
my understanding.

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

It's a little sad not being able to reuse command tag keywords, but it's
even more sad to impact the rest of the query parsing. IIRC you had some
performance test patch with a split of the main parser into queries and
dml on the one hand, and utility commands on the other hand. Would that
help here? (I mean more as a general solution against that bloat problem
than for this very patch here).

I prefer the solution of using 'ALTER TABLE' rather than ALTER TABLE,
even if code wise we're not gaining anything in complexity: the parser
bloat gets replaced by a big series of if branches. Of course you only
exercise it when you need to. I will change that for next patch.

>>> 3. The event trigger cache seems to be a few bricks shy of a load.
>
> 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 just done rewriting the cache management with a catalog cache for
event triggers and a Syscache Callback that calls into a new module
called src/backend/utils/cache/evtcache.c that mimics attoptcache.c. No
more cache stale variable. And a proper composite hash key.

I still have some more work here before being able to send a new release
of the patch, as I said I won't have enough time to make that happen
until within next week. The git repository is updated, though.
 https://github.com/dimitri/postgres/tree/evt_trig_v1
https://github.com/dimitri/postgres/compare/913091de51...861eb038d0

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