Re: Event Triggers reduced, v1

Hi,

Robert Haas <robertmhaas@gmail.com> writes:
> That's not quite what I was thinking.  I actually can't imagine any
> situation where you want an event trigger that gets fired on EVERY
> command for which we can support command_start.  If you're trying to

I don't have any solid use case in mind, just though it would make the
feature rather complete. Withdrawn.

> So my proposal for the present patch would be:
>
> 1. Rename command_start to ddl_command_start.
> 2. Remove support for everything other than CREATE, ALTER, and DROP.
> 3. Pass the operation and the SQL object type as separate magic variables.

All done in the attached. As usual, you get an incremental patch and a
complete one. It's also published on github for easy browsing:

  https://github.com/dimitri/postgres/compare/33f285e67f...ba42279924

> Yep, sure.  Note that the proposal above constrains the list of
> commands we support in v1 in a very principled way: CREATE, ALTER,
> DROP.  Everything else can be added later under a different (but
> similarly situated) firing point name.  If we stick with command_start
> then I think we're going to be forever justifying our decisions as to
> what got included or excluded; which might be worth it if it seemed
> likely that there'd be much use for such a command trigger, but it
> doesn't (to me, anyway).

Yeah, done that way. I had to remove support for only 4 commands…

> I'm imagining that ddl_command_start triggers would get the
> information this way, but LOAD might be covered by something like
> admin_command_start that just gets the command tag.

My point was that I didn't want to replace the command tag by the object
type and operation combo, happy to see we're on the same line.

> EventTriggerCommandTagsEntry mapping the command tag to an ETC_*
> constant; I think the need for that hash goes away entirely if you
> just pass this information down from the ProcessUtility() switch.  At

Well, no, because we use that hash mapping in BuildEventTriggerCache(),
when reading from the catalogs. I don't see a way to cut down on the
number of per-session hash-table here without involving a penalty.

> any rate having NameData involved seems like it's probably not too
> good an idea; if for some reason we need to keep that hash, use a
> NUL-terminated string and initialize the hash table with string_hash
> instead of tag_hash.  That'll be simpler and also allows the length of
> the buffer to vary independently of NAMEDATALEN, which can only be to
> the good.

Oh, I just failed to realize that the hash table key mustn't be a static
length field. I'm done for tonight though, it's still something to fix.
Maybe you will beat me to it? :) (if not, I'll be happy to, with some
luck even tomorrow).

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