Re: Event Triggers reduced, v1

On Sun, Jun 24, 2012 at 5:46 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)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