Re: Event Triggers reduced, v1

On Fri, Jul 6, 2012 at 7:21 AM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Attached is a incremental patch with a bunch of minor cleanups,
>> including reverts of a few spurious white space changes. Could you
>> merge this into your version?
>
> Thank you very much for that, yes it's included now. So you have 3
> attachments here, the whole new patch revision (v1.7), the incremental
> patch to go from 1.6 to 1.7 and the incremental patch that should apply
> cleanly on top of your cleanups.

Here is an incremental documentation patch which I hope you will like.
I made the event triggers stuff its own chapter rather than trying to
fold it in under triggers, and added some more detail. It's already
quite a bit of extra stuff, and it's only going to become more as we
expand this feature, so I think a separate chapter is appropriate. I
moved a bunch of the details that were under CREATE EVENT TRIGGER into
this new chapter, which I think is a better location, and reformatted
the matrix somewhat. I think as we add more firing points it will be
clearer and easier to read if we have all the commands arranged in
columns rather than listing a bunch of firing points on each line. I
also made a bunch of minor edits to improve readability and improve
the English (which wasn't bad, but I touched it up a bit); and I tried
to add some extra detail here and there (some of it recycled from
previous patch versions). Assuming this all seems reasonably
agreeable, can you merge it on your side?

This took the last several hours, so I haven't looked at your latest
code changes yet. However, in the course of editing the
documentation, it occurred to me that we seem to be fairly arbitrarily
excluding a large number of commands from the event trigger mechanism.
For example, GRANT and REVOKE. In earlier patches, we needed
specific changes for every command, so there was some reason not to
try to support everything right out of the gate. But ISTM that the
argument for this is much less now; presumably it's just a few extra
lines of code per command, so maybe we ought to go ahead and try to
make this as complete as possible. I attempt to explain in the
attached patch the reasons why we don't support certain classes of
commands, but I can't come up with any explanation for supporting
GRANT and REVOKE that doesn't fall flat. I can't even really see a
reason not to support things like LISTEN and NOTIFY, and it would
certainly be more consistent with the notion of a command_start
trigger to support as many commands as we can.

I had an interesting experience while testing this patch. I
accidentally redefined my event trigger function to something which
errored out. That of course precluded me from using CREATE OR REPLACE
FUNCTION to fix it. This makes me feel rather glad that we decided to
exclude CREATE/ALTER/DROP EVENT TRIGGER from the event trigger
mechanism, else recovery would have had to involve system catalog
hackery.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company