Re: Event Triggers reduced, v1 - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> Here is an incremental documentation patch which I hope you will like.

Definitely, it's better this way. I'm not thrilled with separating it
into its own top level chapter, but I can see how it makes sense indeed.

This part is strange though:

+     A trigger definition can also specify a <literal>WHEN</literal>
+     condition so that, for example, a <literal>command_start</literal>
+     tag can be fired only for particular commands which the user wishes
+     to intercept.  A common use of such triggers is to restrict the range of
+     DDL operations which users may perform.

I don't think of that as firing a command tag, so it's hard for me to
parse that sentence.

> 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

+1

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

Done, thanks !

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

As many as that? I'm surprised about the quantity. Yes I did not add all
and any command we have, on purpose, and I agree that the new turn of
things allow us to add a new set.

>  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

Will do soon™.

> 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 would think that NOTIFY is on a fast track not to be disturbed by
calling into used defined code, and that would explain why we don't
support event triggers here.

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

Yeah, we have some places were it's not very hard to shoot oneself in
the foot, here the resulting hole is a little too big and offers no real
benefits. Event triggers on create|alter|drop event triggers, really?

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