Re: Event Triggers reduced, v1

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