Re: Event Triggers reduced, v1
От | Dimitri Fontaine |
---|---|
Тема | Re: Event Triggers reduced, v1 |
Дата | |
Msg-id | m28vf2z200.fsf@2ndQuadrant.fr обсуждение исходный текст |
Ответ на | Re: Event Triggers reduced, v1 (Dimitri Fontaine <dimitri@2ndQuadrant.fr>) |
Ответы |
Re: Event Triggers reduced, v1
(Thom Brown <thom@linux.com>)
Re: Event Triggers reduced, v1 (Robert Haas <robertmhaas@gmail.com>) |
Список | pgsql-hackers |
Dimitri Fontaine <dimitri@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 <robertmhaas@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. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Вложения
В списке pgsql-hackers по дате отправления: