Skip site navigation (1) Skip section navigation (2)

Re: Event Triggers reduced, v1

From: Thom Brown <thom(at)linux(dot)com>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Event Triggers reduced, v1
Date: 2012-07-02 15:11:23
Message-ID: CAA-aLv6N=rffJgD7oXUQR9FNDF33=mf5oj2AGYgRW4OPoNx3RQ@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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

Attachment: evt_trig_v1_changes.patch
Description: application/octet-stream (10.4 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2012-07-02 15:17:19
Subject: Re: small bug on 3-digit years in 9.2-dev
Previous:From: Tom LaneDate: 2012-07-02 15:10:45
Subject: Re: How to extend server side encoding GBK

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group