Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
