Re: Event Triggers reduced, v1

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.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support