Event Triggers reduced, v1

On 15 June 2012 21:27, Dimitri Fontaine <dfontaine@hi-media.com> wrote:

The goal for this first patch is to avoid semantics issues so that we
can get something technically clean in, and have more time to talk
semantics next times. The main discussion to avoid is deciding if we
want to fire event triggers for CREATE SEQUENCE and CREATE INDEX in a
command that just did implement a SERIAL PRIMARY KEY in a table.

So this patch triggers once per top level command, just with
information about the set of nested events?

I'm happy if we make sweeping initial points like "don't generate
events for sequences and indexes" in the first version. We can always
add more later.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Dimitri Fontaine's message of vie jun 15 16:27:50 -0400 2012:

The attached patch contains all the infrastructure for event triggers
and also a first implementation of them for the event "command_start",
implemented in a single place in utility.c.

The infrastructure is about:

- new catalog
- grammar for new commands
- documentation skeleton
- pg_dump support
- psql support
- ability to actually run user triggers
- written in "core languages"
(pl/c, pl/pgsql, pl/python, pl/perl, pl/tcl)
- limited subcommand handling

Did you try REASSIGN OWNED and DROP OWNED with a role that has defined
some event triggers?

Look, it's an easy little skinny patch to review, right:

git --no-pager diff --shortstat master
62 files changed, 4546 insertions(+), 108 deletions(-)

Skinny ... right. I started to give it a look -- I may have something
useful to comment later.

This patch includes regression tests that we worked on with Thom last
rounds, remember that they only run in the serial schedule, that means
with `make installcheck` only. Adding noisy output at random while the
parallel schedule run is a good way to break all the regression testing,
so I've been avoiding that.

Hmm, I don't like the idea of a test that only runs in serial mode.
Maybe we can find some way to make it work in parallel mode as well.
I don't have anything useful to comment right now.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Jun 15, 2012 at 4:27 PM, Dimitri Fontaine
<dfontaine@hi-media.com> wrote:

Allow me to open the new season of the DML trigger series, named
pg_event_trigger. This first episode is all about setting up the drama,
so that next ones make perfect sense.

Comments:

1. I still think we ought to get rid of the notion of BEFORE or AFTER
(i.e. pg_event_trigger.evttype) and just make that detail part of the
event name (e.g. pg_event_trigger.evtevent). Many easily forseeable
event types will be more like "during" rather than "before" or
"after", and for those that do have a notion of before and after, we
can have two different event names and include the word "before" or
"after" there. I am otherwise satisfied with the schema you've
chosen.

2. I think it's important to be able to add new types of event
triggers without creating excessive parser bloat. I think it's
important to use some kind of generic syntax here which will be able
to apply to all types of triggers we may want to add, now or in the
future. The easiest way to do that is to use literal syntax for the
list of command tags, rather than writing them out as key words: e.g.
'ALTER TABLE' rather than ALTER TABLE. It's not quite as pretty, but
the savings in parser bloat and future code change seem well worth it.
Or, alternatively, we could use identifier style, e.g. alter_table,
as I previously suggested.

3. The event trigger cache seems to be a few bricks shy of a load.
First, event_trigger_cache_is_stalled is mis-named; I think you mean
"stale", not "stalled". Second, instead of setting that flag and then
rebuilding the cache when you see the flag set, how about just blowing
away the cache contents whenever you would have set the flag? That
seems a whole lot simpler and cleaner, and removes the need for a
force_rebuild flag on BuildEventTriggerCache(). Third, ISTM that this
isn't going to work correctly if backend A performs an event after
backend B has built its cache. To fix this, I think you need to rip
out all the places where you force a rebuild locally and instead use
something like CacheRegisterSyscacheCallback() to blow away the cache
whenever something changes; you might find it helpful to look at
attoptcache.c.

4. The documentation doesn't build.

openjade:reference.sgml:44:4:W: cannot generate system identifier for
general entity "alterEventTrigger"
openjade:reference.sgml:44:4:E: general entity "alterEventTrigger" not
defined and no default entity
openjade:reference.sgml:44:21:E: reference to entity
"alterEventTrigger" for which no system identifier could be generated
openjade:reference.sgml:44:3: entity was defined here
openjade:reference.sgml:86:4:W: cannot generate system identifier for
general entity "createEventTrigger"
openjade:reference.sgml:86:4:E: general entity "createEventTrigger"
not defined and no default entity
openjade:reference.sgml:86:22:E: reference to entity
"createEventTrigger" for which no system identifier could be generated
openjade:reference.sgml:86:3: entity was defined here
openjade:reference.sgml:125:4:W: cannot generate system identifier for
general entity "dropEventTrigger"
openjade:reference.sgml:125:4:E: general entity "dropEventTrigger" not
defined and no default entity
openjade:reference.sgml:125:20:E: reference to entity
"dropEventTrigger" for which no system identifier could be generated
openjade:reference.sgml:125:3: entity was defined here
openjade:catalogs.sgml:1868:35:E: character "_" is not allowed in the
value of attribute "ZONE"
openjade:catalogs.sgml:1868:19:X: reference to non-existent ID
"CATALOG-PG-EVENT_TRIGGER"
openjade:trigger.sgml:43:47:X: reference to non-existent ID
"SQL-CREATECOMMANDTRIGGER"

5. In terms of a psql command, I think that \dev is both not very
mnemonic and, as you mentioned in the comment, possibly confusable
with SQL/MED. If we're going to have a \d command for this, I suggest
something like \dy, which is not very mnemonic either but at least
seems unlikely to be confused with anything else. Other things that
are unused include \dh, \dj, \dk, \dq, \dw, and \dz, if any of those
grab you, or a somewhat broader range of things (but still nothing
very memorable) if we include capital letters. Or we could branch out
into punctuation, like \d& -- or things that don't begin with the
letter d, but that doesn't seem like a particularly good idea.

6. In objectaddress.c, I think that get_object_address_event_trigger
should be eliminated in favor of an additional case in
get_object_address_unqualified.

7. There are many references to command triggers that still need to be
cleaned up. trigger.sgml still makes reference to the name command
triggers. plpgsql.sgml also contains vestiges of the command trigger
notation, and references to some TG_* variables that I don't think
exist in this version of the patch. event_trigger.c is identified
(twice) as cmdtrigger.c in the file header comment. The header
comment for InsertEventTriggerTuple makes reference to pg_cmdtrigger,
as does a line comment in RemoveEventTriggerById. The regression
output mentions cmdtrigger in a few places as well. In the
documentation, event triggers are mentioned as having return type
command_trigger, but it's now called event_trigger.

8. The re-addition of node tags like T_RemoveFuncStmt seems to be a
merging error. Changing \dc so that it rejects \dcrap appears to be a
leftover from when the command was \dcT. In one place in the docs you
have 'avent' for 'event'. In event_trigger.c, you have #ifdef
UNDEFINED; project standard is #ifdef NOT_USED (or else you can remove
the code).

9. The regression tests seem to now be testing some features that
don't exist any more, and might need some rethinking to make what they
do match the scope of this patch.

10. I suggest separating out the support for other PLs (Python, Tcl)
and submitting that as a later patch, since I'm unqualified to commit
it (and I'm hoping to get the rest of this committed). The PL/TCL
stuff also contains residual references to the command-trigger
notation which should be cleaned up before resubmitting.

There's probably more, but I'm all reviewed out for right now.
Hopefully that's enough to get you started. I think this is heading
in a good direction, even though there's still a good bit of work left
to do.

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

Did you try REASSIGN OWNED and DROP OWNED with a role that has defined
some event triggers?

Not yet. Will add to regression tests, good catch.

Hmm, I don't like the idea of a test that only runs in serial mode.
Maybe we can find some way to make it work in parallel mode as well.

I don't see how we can manage to do that, as adding an event trigger to
some command will impact tests running in parallel.

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

On Wednesday, June 20, 2012 09:33:26 PM Dimitri Fontaine wrote:

Hmm, I don't like the idea of a test that only runs in serial mode.
Maybe we can find some way to make it work in parallel mode as well.

I don't see how we can manage to do that, as adding an event trigger to
some command will impact tests running in parallel.

Cant you just put it alone in a test group in the parallel_schedule? Several
other tests do that?

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jun 20, 2012 at 3:39 PM, Andres Freund <andres@2ndquadrant.com> wrote:

On Wednesday, June 20, 2012 09:33:26 PM Dimitri Fontaine wrote:

Hmm, I don't like the idea of a test that only runs in serial mode.
Maybe we can find some way to make it work in parallel mode as well.

I don't see how we can manage to do that, as adding an event trigger to
some command will impact tests running in parallel.

Cant you just put it alone in a test group in the parallel_schedule? Several
other tests do that?

Yeah, that seems like it should work. If not, I'd say the tests
themselves are broken.

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

Hi,

Robert Haas <robertmhaas@gmail.com> writes:

1. I still think we ought to get rid of the notion of BEFORE or AFTER
(i.e. pg_event_trigger.evttype) and just make that detail part of the
event name (e.g. pg_event_trigger.evtevent). Many easily forseeable
event types will be more like "during" rather than "before" or
"after", and for those that do have a notion of before and after, we
can have two different event names and include the word "before" or
"after" there. I am otherwise satisfied with the schema you've
chosen.

It's not before/after anymore, but rather addon/replace if you will. I
kept the INSTEAD OF keyword for the replace semantics, that you've been
asking me to keep IIRC, with security policy plugins as a use case.

Now we can of course keep those semantics and embed them in the event
name we provide users, I though that maybe a documentation matrix of
which event support which "mode" would be cleaner to document. We might
as well find a clean way to implement both modes for most of the
commands, I don't know yet.

So, are you sure you want to embed that part of the event trigger
semantics in the event name itself?

2. I think it's important to be able to add new types of event
triggers without creating excessive parser bloat. I think it's

I've been trying to do that yes, as you can see with event_name and
event_trigger_variable rules. I've been re-using as much existing
keywords as I could because I believe that's not causing any measurable
bloat, I'll kindly reconsider if necessary, even if sadly.

important to use some kind of generic syntax here which will be able
to apply to all types of triggers we may want to add, now or in the
future. The easiest way to do that is to use literal syntax for the
list of command tags, rather than writing them out as key words: e.g.
'ALTER TABLE' rather than ALTER TABLE. It's not quite as pretty, but
the savings in parser bloat and future code change seem well worth it.
Or, alternatively, we could use identifier style, e.g. alter_table,
as I previously suggested.

Whatever the solution here, we still need to be able to ERROR out very
early if the user entered a non supported command here, such as GRANT
for the time being. I'm not sure a manual list of strcmp() would be more
effective than having bison basically produce the same thing for us. I
think using the grammar here makes for easier maintenance.

3. The event trigger cache seems to be a few bricks shy of a load.

I wouldn't be that surprised, mind you. I didn't have nearly as much
time I wanted to working on that project.

First, event_trigger_cache_is_stalled is mis-named; I think you mean
"stale", not "stalled". Second, instead of setting that flag and then

Stale. Right. Edited.

rebuilding the cache when you see the flag set, how about just blowing
away the cache contents whenever you would have set the flag? That

I've been doing that at first, but that meant several full rebuilds in a
row in the regression tests, which are adding new event triggers then
using them. I though lazily maintaining the cache would be better.

seems a whole lot simpler and cleaner, and removes the need for a
force_rebuild flag on BuildEventTriggerCache(). Third, ISTM that this
isn't going to work correctly if backend A performs an event after
backend B has built its cache. To fix this, I think you need to rip
out all the places where you force a rebuild locally and instead use
something like CacheRegisterSyscacheCallback() to blow away the cache
whenever something changes; you might find it helpful to look at
attoptcache.c.

Ok, looking at that for next revision of the patch, which I should be
able to post early next week.

4. The documentation doesn't build.

Sorry about that, will get fixed too.

5. In terms of a psql command, I think that \dev is both not very
mnemonic and, as you mentioned in the comment, possibly confusable
with SQL/MED. If we're going to have a \d command for this, I suggest
something like \dy, which is not very mnemonic either but at least
seems unlikely to be confused with anything else. Other things that
are unused include \dh, \dj, \dk, \dq, \dw, and \dz, if any of those
grab you, or a somewhat broader range of things (but still nothing
very memorable) if we include capital letters. Or we could branch out
into punctuation, like \d& -- or things that don't begin with the
letter d, but that doesn't seem like a particularly good idea.

I'm not that fond of psql commands, but I don't think it's going to fly
not to have one for event triggers. I could buy \dy.

6. In objectaddress.c, I think that get_object_address_event_trigger
should be eliminated in favor of an additional case in
get_object_address_unqualified.

Sure. It used to be more complex than that when the identifier was a
composite with the command name, it makes no sense to separate it away
now. Done.

7. There are many references to command triggers that still need to be
cleaned up. trigger.sgml still makes reference to the name command
triggers. plpgsql.sgml also contains vestiges of the command trigger
notation, and references to some TG_* variables that I don't think
exist in this version of the patch. event_trigger.c is identified
(twice) as cmdtrigger.c in the file header comment. The header
comment for InsertEventTriggerTuple makes reference to pg_cmdtrigger,
as does a line comment in RemoveEventTriggerById. The regression
output mentions cmdtrigger in a few places as well. In the
documentation, event triggers are mentioned as having return type
command_trigger, but it's now called event_trigger.

All fixed, will grep for cmd and command in the patch and fix any other
one that's still there before sending v2 of the patch. Sorry about that.

8. The re-addition of node tags like T_RemoveFuncStmt seems to be a
merging error. Changing \dc so that it rejects \dcrap appears to be a
leftover from when the command was \dcT. In one place in the docs you
have 'avent' for 'event'. In event_trigger.c, you have #ifdef
UNDEFINED; project standard is #ifdef NOT_USED (or else you can remove
the code).

Will see about node tags and psql clean merge. Docs fixed. I meant to
remove the code. Done now. Thanks.

9. The regression tests seem to now be testing some features that
don't exist any more, and might need some rethinking to make what they
do match the scope of this patch.

The current implementation must be kicking for all supported commands
and we have a authoritative list of them in the grammar, so I wanted to
maintain a regression test suite where they are all exercised, even if
we're exercising the same code path each time.

That's meant to change with later patch, I'm not sure how much gain we
have to remove test covering now knowing that we certainly won't release
with only that first patch.

10. I suggest separating out the support for other PLs (Python, Tcl)
and submitting that as a later patch, since I'm unqualified to commit
it (and I'm hoping to get the rest of this committed). The PL/TCL
stuff also contains residual references to the command-trigger
notation which should be cleaned up before resubmitting.

Fixed pltcl internal references. Will produce separate patches for next
revision.

There's probably more, but I'm all reviewed out for right now.
Hopefully that's enough to get you started. I think this is heading
in a good direction, even though there's still a good bit of work left
to do.

Thanks for your review, it's clearly enough to get started chewing on
the patch!

Early followers can see the progress, when it happens, in the github
repository, if waiting for the next patch is unbearably long :)

https://github.com/dimitri/postgres
https://github.com/dimitri/postgres/tree/evt_trig_v1

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

Robert Haas <robertmhaas@gmail.com> writes:

Cant you just put it alone in a test group in the parallel_schedule? Several
other tests do that?

Yeah, that seems like it should work. If not, I'd say the tests
themselves are broken.

I completely missed that we could do that. I don't feel bright. Of
course it just works.

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

On Wed, Jun 20, 2012 at 4:36 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

1. I still think we ought to get rid of the notion of BEFORE or AFTER
(i.e. pg_event_trigger.evttype) and just make that detail part of the
event name (e.g. pg_event_trigger.evtevent).  Many easily forseeable
event types will be more like "during" rather than "before" or
"after", and for those that do have a notion of before and after, we
can have two different event names and include the word "before" or
"after" there.  I am otherwise satisfied with the schema you've
chosen.

It's not before/after anymore, but rather addon/replace if you will. I
kept the INSTEAD OF keyword for the replace semantics, that you've been
asking me to keep IIRC, with security policy plugins as a use case.

Now we can of course keep those semantics and embed them in the event
name we provide users, I though that maybe a documentation matrix of
which event support which "mode" would be cleaner to document. We might
as well find a clean way to implement both modes for most of the
commands, I don't know yet.

So, are you sure you want to embed that part of the event trigger
semantics in the event name itself?

Yeah, pretty sure. I think that for regular triggers, BEFORE, AFTER,
and INSTEAD-OF are the firing-point specification. But even triggers
will have more than three firing points, probably eventually quite a
lot more. So we need something more flexible. But we don't need that
more flexible thing AND ALSO the before/after/instead-of
specification, which I think in most cases won't be meaningful anyway.
It happens to be somewhat sensible for this initial firing point, but
I think for most of them there will be just one place, and in many
cases it will be neither before, nor after, nor instead-of.

2. I think it's important to be able to add new types of event
triggers without creating excessive parser bloat.  I think it's

I've been trying to do that yes, as you can see with event_name and
event_trigger_variable rules. I've been re-using as much existing
keywords as I could because I believe that's not causing any measurable
bloat, I'll kindly reconsider if necessary, even if sadly.

The issue is that the size of the parser tables grow with the square
of the number of states. This will introduce lots of new states that
we don't really need; and every new kind of event trigger that we want
to add will introduce more.

3. The event trigger cache seems to be a few bricks shy of a load.

I wouldn't be that surprised, mind you. I didn't have nearly as much
time I wanted to working on that project.

First, event_trigger_cache_is_stalled is mis-named; I think you mean
"stale", not "stalled".  Second, instead of setting that flag and then

Stale. Right. Edited.

rebuilding the cache when you see the flag set, how about just blowing
away the cache contents whenever you would have set the flag?  That

I've been doing that at first, but that meant several full rebuilds in a
row in the regression tests, which are adding new event triggers then
using them. I though lazily maintaining the cache would be better.

Well, AFAICS, you're still doing full rebuilds whenever something
changes; you're just keeping the (useless, dead) cache around until
you decide to rebuild it. Might as well free the memory once you know
that the next access will rebuild it anyway, and for a bonus it saves
you a flag.

I'm not that fond of psql commands, but I don't think it's going to fly
not to have one for event triggers. I could buy \dy.

Yeah, I think people are going to want to have one. I really despise
the \d<whatever> syntax, but it's not 100% clear what a better one
would look like.

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

Robert Haas <robertmhaas@gmail.com> writes:

It's not before/after anymore, but rather addon/replace if you will. I
kept the INSTEAD OF keyword for the replace semantics, that you've been
asking me to keep IIRC, with security policy plugins as a use case.

Now we can of course keep those semantics and embed them in the event
name we provide users, I though that maybe a documentation matrix of
which event support which "mode" would be cleaner to document. We might
as well find a clean way to implement both modes for most of the
commands, I don't know yet.

So, are you sure you want to embed that part of the event trigger
semantics in the event name itself?

Yeah, pretty sure. I think that for regular triggers, BEFORE, AFTER,
and INSTEAD-OF are the firing-point specification. But even triggers
will have more than three firing points, probably eventually quite a
lot more. So we need something more flexible. But we don't need that
more flexible thing AND ALSO the before/after/instead-of
specification, which I think in most cases won't be meaningful anyway.
It happens to be somewhat sensible for this initial firing point, but
I think for most of them there will be just one place, and in many
cases it will be neither before, nor after, nor instead-of.

I agree with using the event name as a the specification for the firing
point, and that we should prefer documenting the ordering of those
rather than offering a fuzzy idea of BEFORE and AFTER steps in there.
The AFTER step is better expressed as BEFORE the next one.

Now, I still think there's an important discrepancy between adding a new
behaviour that adds-up to whatever the backend currently implements and
providing a replacement behaviour with a user defined function that gets
called instead of the backend code.

And I still don't think that the event name should be carrying alone
that semantic discrepancy. Now, I also want the patch to get in, so I
won't insist very much if I'm alone in that position. Anyone else
interested enough to chime in?

The user visible difference would be between those variants:

create event trigger foo at 'before_security_check' ...
create event trigger foo at 'replace_security_check' ...

create event trigger foo before 'security_check' ...
create event trigger foo instead of 'security_check' ...

Note that in this version the INSTEAD OF variant is not supported, we
only intend to offer it in some very narrow cases, or at least that is
my understanding.

The issue is that the size of the parser tables grow with the square
of the number of states. This will introduce lots of new states that
we don't really need; and every new kind of event trigger that we want
to add will introduce more.

It's a little sad not being able to reuse command tag keywords, but it's
even more sad to impact the rest of the query parsing. IIRC you had some
performance test patch with a split of the main parser into queries and
dml on the one hand, and utility commands on the other hand. Would that
help here? (I mean more as a general solution against that bloat problem
than for this very patch here).

I prefer the solution of using 'ALTER TABLE' rather than ALTER TABLE,
even if code wise we're not gaining anything in complexity: the parser
bloat gets replaced by a big series of if branches. Of course you only
exercise it when you need to. I will change that for next patch.

3. The event trigger cache seems to be a few bricks shy of a load.

Well, AFAICS, you're still doing full rebuilds whenever something
changes; you're just keeping the (useless, dead) cache around until
you decide to rebuild it. Might as well free the memory once you know
that the next access will rebuild it anyway, and for a bonus it saves
you a flag.

I'm just done rewriting the cache management with a catalog cache for
event triggers and a Syscache Callback that calls into a new module
called src/backend/utils/cache/evtcache.c that mimics attoptcache.c. No
more cache stale variable. And a proper composite hash key.

I still have some more work here before being able to send a new release
of the patch, as I said I won't have enough time to make that happen
until within next week. The git repository is updated, though.

https://github.com/dimitri/postgres/tree/evt_trig_v1
https://github.com/dimitri/postgres/compare/913091de51...861eb038d0

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

Hi,

Here's an early revision 2 of the patch, not yet ready for commit, so
including the PL stuff still. What's missing is mainly a cache reference
leak to fix at DROP EVENT TRIGGER, but I failed to spot where it comes
from.

As I fixed about all the other comments I though I might as well send in
the new version of the patch to get to another round of review.

Robert Haas <robertmhaas@gmail.com> writes:

1. I still think we ought to get rid of the notion of BEFORE or AFTER
(i.e. pg_event_trigger.evttype) and just make that detail part of the
event name (e.g. pg_event_trigger.evtevent). Many easily forseeable

So, agreed on before/after, not on INSTEAD OF. No change in the patch,
still discussing that point.

2. I think it's important to be able to add new types of event
triggers without creating excessive parser bloat. I think it's

Fixed in the attached, I believe.

3. The event trigger cache seems to be a few bricks shy of a load.

Fixed in the attached, including cache invalidation registered as a
system cache callback.

4. The documentation doesn't build.

Fixed, I mainly managed to forget adding the new files.

5. In terms of a psql command, I think that \dev is both not very

Switched to \dy and cleaned up.

6. In objectaddress.c, I think that get_object_address_event_trigger
should be eliminated in favor of an additional case in
get_object_address_unqualified.

Fixed in the attached.

7. There are many references to command triggers that still need to be
cleaned up.

All fixed.

8. The re-addition of node tags like T_RemoveFuncStmt seems to be a
merging error. Changing \dc so that it rejects \dcrap appears to be a
leftover from when the command was \dcT. In one place in the docs you
have 'avent' for 'event'. In event_trigger.c, you have #ifdef
UNDEFINED; project standard is #ifdef NOT_USED (or else you can remove
the code).

All fixed.

9. The regression tests seem to now be testing some features that
don't exist any more, and might need some rethinking to make what they
do match the scope of this patch.

Actually those tests helped me spot some strange things when cleaning up
the cache key, and only some commands would fail. So I'm in favor of
keeping it all for now.

10. I suggest separating out the support for other PLs (Python, Tcl)
and submitting that as a later patch, since I'm unqualified to commit
it (and I'm hoping to get the rest of this committed). The PL/TCL
stuff also contains residual references to the command-trigger
notation which should be cleaned up before resubmitting.

That's for next turn.

There's probably more, but I'm all reviewed out for right now.
Hopefully that's enough to get you started. I think this is heading
in a good direction, even though there's still a good bit of work left
to do.

So, let's see about that remaining bit of work :)

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

On Sun, Jun 24, 2012 at 5:46 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Here's an early revision 2 of the patch, not yet ready for commit, so
including the PL stuff still. What's missing is mainly a cache reference
leak to fix at DROP EVENT TRIGGER, but I failed to spot where it comes
from.

As I fixed about all the other comments I though I might as well send in
the new version of the patch to get to another round of review.

Some of the pg_dump hunks fail to apply for me; I guess this needs to
be remerged.

Spurious hunk:

-    query_hosts
+    query_hosts

Spurious hunk:

- * need deep copies, so they should be copied via list_copy()
+ * need deep copies, so they should be copied via list_copy(const )

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.

Incidentally, why do we not support an argument list here, as with
ordinary triggers?

I think the below hunk should get removed. Or else it should be
surrounded by #ifdef rather than commented out.

+       /*
+        * Useful to raise WARNINGs for any DDL command not yet supported.
+        *
+       elog(WARNING, "Command Tag:    %s", tag);
+       elog(WARNING, "Note to String: %s", nodeToString(parsetree));
+        */

Typo:

+ * where we probide object name and namespace separately and still want nice

Typo:

+ * the easier code makes up fot it big time.

psql is now very confused about what the slash command is. It's
actually implemented as \dy, but the comments say \dev in several
places, and slashUsage() documents it as \dct.

InitializeEvtTriggerCommandCache still needs work. First, it ensures
that CacheMemoryContext is non-NULL... after it's already stored the
value of CacheMemoryContext into the HASHCTL. Obviously, the order
there needs to be fixed. Also, I think you need to split this into
two functions. There should be one function that gets called just
once at startup time to CacheRegisterSyscacheCallback(), because we
don't want to redo that every time the cache gets blown away. Then
there should be another function that gets called when, and only when,
someone needs to use the cache. That should create and populate the
hash table.

I think that all event triggers should fire in exactly alphabetical
order by name. Now that we have the concept of multiple firing
points, there's really no reason to distinguish between any triggers
and triggers on specific commands. Eliminating that distinction will
eliminate a bunch of complexity while making things *more* flexible
for the user, who will be able to get a command trigger for a specific
command to fire either before or after an ANY command trigger he has
also defined rather than having the system enforce policy on him.

plperl_validator still makes reference to CMDTRIGGER.

Let's simplify this down to an enum with just one element, since
that's all we're going to support for starters, and remove the related
parser support for everything but command_start:

+typedef enum TrigEvent
+{
+       E_CommandStart       = 1,
+       E_SecurityCheck      = 10,
+       E_ConsistencyCheck   = 15,
+       E_NameLookup         = 20,
+       E_CommandEnd         = 51
+} TrigEvent;

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).

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.

It seems to me that we probably need a CommandCounterIncrement() after
firing each event trigger, unless that's happening under the covers
somewhere and I'm missing it. A good test case would be to have two
event triggers. Have the first one insert a row into the table and
check that the second one can see the row there. I suggest adding
something like this to the regression tests.

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. :-)

The comment changes in type_sanity.sql seem unnecessary. I think you
can revert that part. There's also a one-line whitespace change in
triggers.sql which can also be removed.

With respect to this hunk, it seems to me that the verbiage is
inaccurate. It will forbid the execution of any command for which
event triggers are supported, whether they happen to be DDL commands
or not. Also, a hypothetical DDL command that can't tolerate event
triggers wouldn't be forbidden. I'm not sure exactly what the wording
should look like here, but we should strive to be as exact as
possible.

+  <para>
+   Forbids the execution of any DDL command:
+
+<programlisting>
+CREATE OR REPLACE FUNCTION abort_any_command()
+  RETURNS event_trigger
+ LANGUAGE plpgsql
+  AS $$
+BEGIN
+  RAISE EXCEPTION 'command % is disabled', tg_tag;
+END;
+$$;

format_type_be_without_namespace is unused in this patch; please
remove it for now.

get_event_trigger_oid looks like it might be the source of your
syscache reference leak. It would be a good idea to change the coding
pattern of this function to match, say, get_foreign_server_oid. That
would fix the leak and be more consistent.

I'm all reviewed out; hope that's enough for now. I hope you can get
this cleaned up some more soon, as we are starting to run out of
CommitFest and I would really like to get this in. Of course if we
miss the CommitFest deadline I am happy to work on it in July and
August but the sooner we get it done the better.

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

On Thu, Jun 28, 2012 at 9:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

[ review ]

Also:

../../../src/include/catalog/pg_event_trigger.h:34: error: expected
specifier-qualifier-list before ‘int2’

This needs to be changed to int16 as a result of commit
b8b2e3b2deeaab19715af063fc009b7c230b2336.

alter.c:73: warning: implicit declaration of function ‘RenameEventTrigger’

That file needs to include commands/event_trigger.h.

Please spell out DO_EVTTRIGGER as DO_EVENT_TRIGGER.

Another random warning:

plpy_main.c:195: warning: ‘retval’ may be used uninitialized in this function

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

Hi,

Here's an answer to your review (thanks!), with no patch attached yet
even if I've been cleanup up most of what you reported. Incremental diff
is available for browsing here:

https://github.com/dimitri/postgres/compare/f99e8d93b7...8da156dc70

Robert Haas <robertmhaas@gmail.com> writes:

Some of the pg_dump hunks fail to apply for me; I guess this needs to
be remerged.

Done.

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.

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?

I think the below hunk should get removed. Or else it should be
surrounded by #ifdef rather than commented out.

Removed.

Typo:

Fixed.

psql is now very confused about what the slash command is. It's
actually implemented as \dy, but the comments say \dev in several
places, and slashUsage() documents it as \dct.

Fixed.

InitializeEvtTriggerCommandCache still needs work. First, it ensures
that CacheMemoryContext is non-NULL... after it's already stored the
value of CacheMemoryContext into the HASHCTL. Obviously, the order
there needs to be fixed. Also, I think you need to split this into
two functions. There should be one function that gets called just
once at startup time to CacheRegisterSyscacheCallback(), because we
don't want to redo that every time the cache gets blown away. Then
there should be another function that gets called when, and only when,
someone needs to use the cache. That should create and populate the
hash table.

CacheMemoryContext ordering fixed, I wrongly copied from attoptcache
here even when the memory management is not really done the same. The
only place I can see where to init the Event Trigger Cache is in
InitPostgres() itself (in src/backend/utils/init/postinit.c), done now.

I think that all event triggers should fire in exactly alphabetical
order by name. Now that we have the concept of multiple firing
points, there's really no reason to distinguish between any triggers
and triggers on specific commands. Eliminating that distinction will
eliminate a bunch of complexity while making things *more* flexible
for the user, who will be able to get a command trigger for a specific
command to fire either before or after an ANY command trigger he has
also defined rather than having the system enforce policy on him.

Internally we still need to keep the distinction to be able to fire ANY
triggers on otherwise non supported commands. I agree that we should not
concern our users with that, though. Done.

plperl_validator still makes reference to CMDTRIGGER.

In a comment, fixed.

Let's simplify this down to an enum with just one element, since
that's all we're going to support for starters, and remove the related
parser support for everything but command_start:

+typedef enum TrigEvent
+{
+       E_CommandStart       = 1,
+       E_SecurityCheck      = 10,
+       E_ConsistencyCheck   = 15,
+       E_NameLookup         = 20,
+       E_CommandEnd         = 51
+} TrigEvent;

I wanted to see where the current choice would lead us, I agree that we
can remove that level of details for now, that also allows not to pick
next event integration point names already :) Done.

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?

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.

It seems to me that we probably need a CommandCounterIncrement() after
firing each event trigger, unless that's happening under the covers
somewhere and I'm missing it. A good test case would be to have two
event triggers. Have the first one insert a row into the table and
check that the second one can see the row there. I suggest adding
something like this to the regression tests.

Added CommandCounterIncrement() and a new regression test. That fails
for now, I'll have to get back to that later.

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.

The comment changes in type_sanity.sql seem unnecessary. I think you
can revert that part. There's also a one-line whitespace change in
triggers.sql which can also be removed.

Done.

With respect to this hunk, it seems to me that the verbiage is
inaccurate. It will forbid the execution of any command for which
event triggers are supported, whether they happen to be DDL commands
or not. Also, a hypothetical DDL command that can't tolerate event
triggers wouldn't be forbidden. I'm not sure exactly what the wording
should look like here, but we should strive to be as exact as
possible.

+  <para>
+   Forbids the execution of any DDL command:
+
+<programlisting>
+CREATE OR REPLACE FUNCTION abort_any_command()
+  RETURNS event_trigger
+ LANGUAGE plpgsql
+  AS $$
+BEGIN
+  RAISE EXCEPTION 'command % is disabled', tg_tag;
+END;
+$$;

Yeah, will rework that text. Not this late though, needs more brainpower.

format_type_be_without_namespace is unused in this patch; please
remove it for now.

Done.

get_event_trigger_oid looks like it might be the source of your
syscache reference leak. It would be a good idea to change the coding
pattern of this function to match, say, get_foreign_server_oid. That
would fix the leak and be more consistent.

Fixed meanwhile, that was it, thanks.

I'm all reviewed out; hope that's enough for now. I hope you can get
this cleaned up some more soon, as we are starting to run out of
CommitFest and I would really like to get this in. Of course if we
miss the CommitFest deadline I am happy to work on it in July and
August but the sooner we get it done the better.

So, I've begun working on this already, and I intend to spend more time
on PostgreSQL development from next week on.

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

Robert Haas <robertmhaas@gmail.com> writes:

../../../src/include/catalog/pg_event_trigger.h:34: error: expected
specifier-qualifier-list before ‘int2’

This needs to be changed to int16 as a result of commit
b8b2e3b2deeaab19715af063fc009b7c230b2336.

Done as part of the previous work.

alter.c:73: warning: implicit declaration of function ‘RenameEventTrigger’

Done now.

Please spell out DO_EVTTRIGGER as DO_EVENT_TRIGGER.

Also done as part of the previous email.

Another random warning:
plpy_main.c:195: warning: ‘retval’ may be used uninitialized in this function

Will do a whole warning check pass later. Can you give me your local
Makefile trick to turn them into hard errors again please? :)

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

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

On 2 July 2012 15:11, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

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.

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

On Fri, Jun 29, 2012 at 5:38 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Will do a whole warning check pass later. Can you give me your local
Makefile trick to turn them into hard errors again please? :)

echo COPT=-Werror > src/Makefile.custom

Your latest patch contains a warning about using a variable
uninitialized that seems to indicate that you didn't test this very
carefully: in get_event_triggers, current_any_name is set but not
used.

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

On Mon, Jul 2, 2012 at 1:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jun 29, 2012 at 5:38 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Will do a whole warning check pass later. Can you give me your local
Makefile trick to turn them into hard errors again please? :)

echo COPT=-Werror > src/Makefile.custom

Your latest patch contains a warning about using a variable
uninitialized that seems to indicate that you didn't test this very
carefully: in get_event_triggers, current_any_name is set but not
used.

Make that used but not set. Anyhow, it's broken. :-(

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