Re: Event Triggers reduced, v1

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Event Triggers reduced, v1
Date: 2012-07-11 23:27:25
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


Robert Haas <robertmhaas(at)gmail(dot)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:

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

Dimitri Fontaine PostgreSQL : Expertise, Formation et Support

Attachment Content-Type Size
event_triggers_v1.8--v1.9.patch text/x-patch 72.6 KB
event_triggers_v1.9.patch.gz application/octet-stream 48.7 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Craig Ringer 2012-07-12 01:23:16 Re: DELETE vs TRUNCATE explanation
Previous Message Alexander Korotkov 2012-07-11 23:11:19 Re: SP-GiST for ranges based on 2d-mapping and quad-tree