On 2 July 2012 15:11, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: > Dimitri Fontaine <dimi...@2ndquadrant.fr> writes: >> https://github.com/dimitri/postgres/compare/f99e8d93b7...8da156dc70 > > The revised incremental diff is here: > > https://github.com/dimitri/postgres/compare/f99e8d93b7...74bbeda8 > > And a new revision of the patch is attached to this email. We have some > pending questions, depending on the answers it could be ready for > commit. > >> Robert Haas <robertmh...@gmail.com> writes: >>> There are a few remaining references to EVTG_FIRED_BEFORE / >>> EVTG_FIRED_INSTEAD_OF which should be cleaned up. I suggest writing >>> the grammar as CREATE EVENT TRIGGER name ON event_name EXECUTE... On >>> a related note, evttype is still mentioned in the documentation, also. >>> And CreateEventTrigStmt has a char timing field that can go. >> >> I didn't get the memo that we had made a decision here :) That said it >> will be possible to change our mind and introduce that instead of syntax >> if that's necessary later in the cycle, so I'll go clean up for the >> first commit. > > Done now. > >>> Incidentally, why do we not support an argument list here, as with >>> ordinary triggers? >> >> Left for a follow-up patch. Do you want it already in this one? > > Didn't do that, I though cleaning up all the points here would go first, > please tell me if you want that in the first commit. > >>> I think we should similarly rip out the vestigial support for >>> supplying schema name, object name, and object ID. That's not going >>> to be possible at command_start anyway; we can include that stuff in >>> the patch that adds a later firing point (command end, or consistency >>> check, perhaps). >> >> We got this part of the API fixed last round, so I would prefer not to >> dumb it down in this first patch. We know that we want to add exactly >> that specification later, don't we? > > Didn't change anything here. > >>> I think standard_ProcessUtility should have a shortcut that bypasses >>> setting up the event context if there are no event triggers at all in >>> the entire system, so that we do no extra work unless there's some >>> potential benefit. >> >> Setting up the event context is a very lightweight operation, and >> there's no way to know if the command is going to fire an event trigger >> without having done exactly what the InitEventContext() is doing. Maybe >> what we need to do here is rename it? >> >> Another problem with short cutting it aggressively is what happens if a >> new event trigger is created while the command is in flight. We have yet >> to discuss about that (as we only support a single timing point), but >> doing it the way you propose forecloses any other choice than >> "repeatable read" equivalent where we might want to have some "read >> commited" behaviour, that is fire the new triggers if they appear while >> the command is being run. > > Same, don't see a way to shortcut. > >> Added CommandCounterIncrement() and a new regression test. That fails >> for now, I'll have to get back to that later. > > Of course I just needed to pay attention to the new ordering rules :) > >>> Instead of having giant switch statements match E_WhatEver tags to >>> strings and visca versa, I think it would be much better to create an >>> array someplace that contains all the mappings. Then you can write a >>> convenience function that scans the array for a string and returns the >>> E_WhatEver tag, and another convenience function that scans the array >>> for an E_WhatEver tag and returns the corresponding string. Then all >>> the knowledge of how those things map onto each other is in ONE place, >>> which should make things a whole lot easier in terms of future code >>> maintenance, not to mention minimizing the chances of bugs of >>> oversight in the patch as it stands. :-) >> >> That means that the Enum definition can not jump from a number to >> another non consecutive one, or that we have a very sparse array and >> some way to fill it unknown to me. As those numbers are going to end up >> on disk, we can not ever change them. I though it would be better to >> mimic what we do with the NodeTag definition here. > > I'd like some more input here. > >>> + Forbids the execution of any DDL command: > > Worked out something. Might need some more input.
I'm not clear on this paragraph: Triggers on <literal>ANY</literal> command support more commands than just this list, and will only provide the <literal>command tag</literal> argument as <literal>NOT NULL</literal>. Do you actually mean it will return NULL? I also attach various typo/grammar fixes. -- Thom
evt_trig_v1_changes.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
