Re: Event Triggers reduced, v1

Hi,

Robert Haas <robertmhaas@gmail.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?

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

> important to use some kind of generic syntax here which will be able
> to apply to all types of triggers we may want to add, now or in the
> future.  The easiest way to do that is to use literal syntax for the
> list of command tags, rather than writing them out as key words: e.g.
> 'ALTER TABLE' rather than ALTER TABLE.  It's not quite as pretty, but
> the savings in parser bloat and future code change seem well worth it.
>  Or, alternatively, we could use identifier style, e.g. alter_table,
> as I previously suggested.

Whatever the solution here, we still need to be able to ERROR out very
early if the user entered a non supported command here, such as GRANT
for the time being. I'm not sure a manual list of strcmp() would be more
effective than having bison basically produce the same thing for us. I
think using the grammar here makes for easier maintenance.

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

> seems a whole lot simpler and cleaner, and removes the need for a
> force_rebuild flag on BuildEventTriggerCache().  Third, ISTM that this
> isn't going to work correctly if backend A performs an event after
> backend B has built its cache.  To fix this, I think you need to rip
> out all the places where you force a rebuild locally and instead use
> something like CacheRegisterSyscacheCallback() to blow away the cache
> whenever something changes; you might find it helpful to look at
> attoptcache.c.

Ok, looking at that for next revision of the patch, which I should be
able to post early next week.

> 4. The documentation doesn't build.

Sorry about that, will get fixed too.

> 5. In terms of a psql command, I think that \dev is both not very
> mnemonic and, as you mentioned in the comment, possibly confusable
> with SQL/MED.  If we're going to have a \d command for this, I suggest
> something like \dy, which is not very mnemonic either but at least
> seems unlikely to be confused with anything else.  Other things that
> are unused include \dh, \dj, \dk, \dq, \dw, and \dz, if any of those
> grab you, or a somewhat broader range of things (but still nothing
> very memorable) if we include capital letters.  Or we could branch out
> into punctuation, like \d& -- or things that don't begin with the
> letter d, but that doesn't seem like a particularly good idea.

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.

> 6. In objectaddress.c, I think that get_object_address_event_trigger
> should be eliminated in favor of an additional case in
> get_object_address_unqualified.

Sure. It used to be more complex than that when the identifier was a
composite with the command name, it makes no sense to separate it away
now. Done.

> 7. There are many references to command triggers that still need to be
> cleaned up.  trigger.sgml still makes reference to the name command
> triggers.  plpgsql.sgml also contains vestiges of the command trigger
> notation, and references to some TG_* variables that I don't think
> exist in this version of the patch.  event_trigger.c is identified
> (twice) as cmdtrigger.c in the file header comment.  The header
> comment for InsertEventTriggerTuple makes reference to pg_cmdtrigger,
> as does a line comment in RemoveEventTriggerById.  The regression
> output mentions cmdtrigger in a few places as well.  In the
> documentation, event triggers are mentioned as having return type
> command_trigger, but it's now called event_trigger.

All fixed, will grep for cmd and command in the patch and fix any other
one that's still there before sending v2 of the patch. Sorry about that.

> 8. The re-addition of node tags like T_RemoveFuncStmt seems to be a
> merging error.  Changing \dc so that it rejects \dcrap appears to be a
> leftover from when the command was \dcT. In one place in the docs you
> have 'avent' for 'event'.  In event_trigger.c, you have #ifdef
> UNDEFINED; project standard is #ifdef NOT_USED (or else you can remove
> the code).

Will see about node tags and psql clean merge. Docs fixed. I meant to
remove the code. Done now. Thanks.

> 9. The regression tests seem to now be testing some features that
> don't exist any more, and might need some rethinking to make what they
> do match the scope of this patch.

The current implementation must be kicking for all supported commands
and we have a authoritative list of them in the grammar, so I wanted to
maintain a regression test suite where they are all exercised, even if
we're exercising the same code path each time.

That's meant to change with later patch, I'm not sure how much gain we
have to remove test covering now knowing that we certainly won't release
with only that first patch.

> 10. I suggest separating out the support for other PLs (Python, Tcl)
> and submitting that as a later patch, since I'm unqualified to commit
> it (and I'm hoping to get the rest of this committed).  The PL/TCL
> stuff also contains residual references to the command-trigger
> notation which should be cleaned up before resubmitting.

Fixed pltcl internal references. Will produce separate patches for next
revision.

> There's probably more, but I'm all reviewed out for right now.
> Hopefully that's enough to get you started.  I think this is heading
> in a good direction, even though there's still a good bit of work left
> to do.

Thanks for your review, it's clearly enough to get started chewing on
the patch!

Early followers can see the progress, when it happens, in the github
repository, if waiting for the next patch is unbearably long :)
 https://github.com/dimitri/postgres https://github.com/dimitri/postgres/tree/evt_trig_v1

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