Re: Event Triggers reduced, v1

On Mon, Jul 2, 2012 at 10:11 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> [ new patch ]

I would really like to start committing parts of this, but there are
still a lot of unfinished loose ends in this code. The one that is
most immediately bothering me is related to the syntax you've
implemented, which is:

CREATE EVENT TRIGGER name ON event_name WHEN event_trigger_variable IN
(trigger_command_list) EXECUTE PROCEDURE func_name ()

I've been beating on the issue of generic syntax for a while now, and
that production looks sort of generic, but there are problems when you
dig into it. trigger_command_list is defined in a way that means that
it can never be anything other than a list of tags, which means that
event_trigger_variable can never really be anything other than TAG.
Oops. I think you need to remove the validation of
trigger_command_list from the parser and do that afterwards. In other
words, the parser should be happy if event_trigger_variable is an
ColId and trigger_command_list is a bunch of comma-separated SConst
items. Then, after parsing is complete, the code that actually
implements CREATE EVENT TRIGGER should look at event_trigger_variable
and decide whether it has a legal value and, if so, whether the
associated trigger_command_list is compatible with it. IOW, the
validation should be happening in CreateEventTrigger rather than
gram.y.

There is a related problem in terms of the system catalog
representation which I should have noted previously, but did not. The
system catalog representation has no place to store
event_trigger_variable, and the column that will store
trigger_command_list is called evttags, again implying rather strongly
that these are command tags we're dealing with rather than anything
else. Obviously this could be revised later, but it will be much
easier to add new functionality in subsequent patches if we get the
catalog infrastructure right - or close to right - on the first try,
so it seems worth spending a little bit of time on it. The two
questions that occur to me are:

1. Do we imagine a situation where a given event_name would allow more
than one choice of event_trigger_variable? If so, then
pg_event_trigger needs a place to store the event_trigger_variable.
If not, then it's a noise word, and we should consider simplifying the
syntax to something like:

CREATE EVENT TRIGGER name ON event_name (trigger_command_list) EXECUTE
PROCEDURE func_name ()
or maybe
CREATE EVENT TRIGGER name ON event_name FOR (trigger_command_list)
EXECUTE PROCEDURE func_name ()
or perhaps
CREATE EVENT TRIGGER name ON event_name USING (trigger_command_list)
EXECUTE PROCEDURE func_name ()

2. On a related point, do we anticipate that we might eventually want
to allow filtering by more than one event_trigger_variable in the same
trigger? That is, might we want to do something like this:

CREATE EVENT TRIGGER something ON somevent WHEN thingy IN ('item1',
'item2') AND otherthingy IN ('foo', 'bar') EXECUTE PROCEDURE func_name
()

If so, then how do we imagine that getting stored in the catalog?

Please let me know your thoughts.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company