Re: Event Triggers reduced, v1

Hi,

Please find attached a new revision of the patch, v1.8, and an
incremental diff from the previous one, that includes the patches you
sent me.

Robert Haas <robertmhaas@gmail.com> writes:
> Oh, really?  I thought that was a huge readability improvement.  There
> are some things that are the same between triggers and event triggers,
> but there's an awful lotta stuff that is completely different.

True.

> Oh, that should say "a command_start trigger" rather than "a
> command_start tag".  Good catch.

Fixed in the attached.

> I think we might want to extend the support matrix to include every
> command that appears in sql-commands.html and have either an X if it's
> supported or blank if it's not.  That would make it easier to judge
> how many commands are not supported, not just for us but for users who
> may be trying to answer the same questions we are.

Yes, not done yet, see below.

> Mind you, if I ran the world, this would probably be broken up
> differently: I'd have ddl_command_start covering all the
> CREATE/ALTER/DROP commands and nothing else; and separate firing
> points for anything else I wanted to support.  It's not too late to
> make that change, hint, hint.  But if we're not gonna do that then I

Let's see about doing that. I guess we would have ddl_command_start and
command_start, and I would think that the later is the most generic. So
we would certainly want DDL commands to run first the command_start
event triggers then the ddl_command_start event triggers, whereas a
NOTIFY would only run command_start triggers, and a GRANT command would
run maybe command_start then dcl_command_start triggers?

If that's where we're going to, we can commit as-is and expand later.

> think that we'd better try to cast the net as broadly as reasonably
> possible.  It seems to me that our excuse for not including things
> like UPDATE and DELETE is a bit thin; surely there are people who
> would like a sort of universal trigger that applies to every relation
> in the system.  Of course there are recursion problems there that need
> to be thought long hard about, and no I don't really want to go there
> right now, but I'll bet you a nickle that someone is going to ask why
> it doesn't work that way.

The current reason why we only support 149 SQL commands and variations
is because we want a patch that's easy enough to review and agree on. So
I think we will in the future be able to add new firing point at places
where maybe some discussion is needed.

Such places, in my mind, include the NOTIFY mechanism, DCLs, and global
objects such as databases and tablespaces and roles. I'd be happy to see
event triggers embrace support for those. Maybe in v2 though?

> Another advantage to recasting this as ddl_command_start is that we
> quite easily pass the operation (CREATE, ALTER, DROP) and the named
> object type (TABLE, FUNCTION, CAST) as separate arguments.  I think

That's a good idea. I don't think we should replace the current tag
support with that though, because some commands are harder to stow into
the operation and type model (in supported commands, mainly LOAD).

So I've included partial support for that in the attached patch, in the
simplest way possible, just so that we can see where it leads in term of
using the feature. The next step here is to actually go in each branch
of the process utility switch and manually decorate the command context
with the current operation and objecttype when relevant.

> One other thought: if we're NOT going to do what I suggested above,
> then how about renaming TG_WHEN to TG_EVENT?  Seems like that would
> fit better.

That seems like the right thing to do in either case, done.

> Also: now that the E_WhatEver constants don't get stored on disk, I
> don't think they should live in pg_event_trigger.h any more; can we
> move them to someplace more appropriate?  Possibly make them private
> to event_trigger.c?  And, on a related note, I don't think it's a good

Done, they now live in evtcache.h, where they belong.

> idea to use E_ as a prefix for both the event types and the command
> tags.  It's too short, and hard to grep for, and we don't want the
> same one for both, I think.  How above things like EVT_CommandStart
> for the events and ECT_CreateAggregate for the command tags?

Done this way.

Robert Haas <robertmhaas@gmail.com> writes:
>> I think we might be able to install a static array for the setup where
>> we would find the different elements, and then code up some procedures
>> doing different kind of look ups in that array.
>
> +1.

Done in the attached. Filling that array was… an interesting use case
for Emacs Keyboard Macros spanning 3 different buffers, maintaining it
should be easy enough now.

> Ugh.  Yeah, obviously the most important thing I think is that
> InitEventContext() needs to be lightning-fast, but we don't want
> BuildEventTriggerCache() to be pathologically slow either.

So I've built two new hash tables so that we can either search internal
command enum number by command tag or by parse tree nodeTag and object
type. The hash table is only built when first needed, I didn't add any
trick to only build it when the pg_event_trigger table is empty.

> I think the best thing to do with InitEventContext() might be to get
> rid of it.  It's just a big switch over node tags, and we've already
> got one of those in standard_ProcessUtility.  Maybe every case that

Given that second hash table (EventTriggerCommandNodeCache) we can now
move that look-up to later in the course of events. I kept
InitEventContext() as the place where to stuff default values in the
EventContext structure, though.

Also, as we're optimizing things, there's no longer any call sites to
the function CommandFiresTriggersForEvent() in the attached patch.
That's because this function main use case is allowing the specific
command support code to avoid some possibly costly setup when there's in
fact no trigger to fire. That's never going to be the case with
command_start triggers as they have no specific variable support.

A typical integration now looks like this:

    case T_CreateCastStmt:
        ExecEventTriggers(&evt, EVT_CommandStart);
        CreateCast((CreateCastStmt *) parsetree);
        break;

> Now that leaves the question of how to translate between
> E_AlterAggregate and "ALTER AGGREGATE"; I think your idea of a hash
> table (or two?) might be the most practical option.  We'd only need to
> build the hash table(s) if an index-scan of pg_event_trigger finds it
> non-empty, and then only once per session.

As said earlier, I implemented that without the non-empty trick.

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