Re: Event Triggers reduced, v1

On 2 July 2012 15:11, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)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(at)gmail(dot)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