Re: Add CREATE support to event triggers

Hi,

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> So here's a patch implementing the ideas expressed in this thread.
> There are two new SQL functions:

I spent some time reviewing the patch and tried to focus on a higher
level review, as I saw Andres already began with the lower level stuff.

The main things to keep in mind here are:
 - this patch enables running Event Triggers anytime a new object is   created, in a way that the user code is run once
theobject already   made it through the catalogs; 
 - the Event Trigger code has access to the full details about every   created object, so it's not tied to a command
butreally the fact   that an object just was created in the catalogs; 
      (it's important with serial and primary key sub-commands)
 - facilities are provided so that it's possible to easily build an SQL   command that if executed would create the
exactsame object again; 
 - the facilities around passing the created object details and   building a SQL command are made in such a way that
it'strivially   possible to "hack" away the captured objects properties before   producing again a new SQL command. 

After careful study and thinking, it appears to me that the proposed
patch addresses the whole range of features we expect here, and is both
flexible enough for the users and easy enough to maintain.

The event being fired once the objects are available in the catalogs
makes it possible for the code providing the details in the JSON format
to complete the parsetree with necessary information.

Current state of the patch is not ready for commit yet, independant of
code details some more high-level work needs to be done:
 - preliminary commit
   It might be a good idea to separate away some pre-requisites of this   patch and commit them separately: the OID
publishingparts and   allowing an Event Trigger to get fired after CREATE but without the   extra detailed JSON
formatedinformation might be good commits   already, and later add the much needed details about what did   happen. 
 - document the JSON format
   I vote for going with the proposed format, because it actually   allows to implement both the audit and replication
featureswe want,   with the capability of hacking schema, data types, SQL   representation etc; and because I couldn't
thinkof anything better   than what's proposed here ;-) 
 - other usual documentation
   I don't suppose I have to expand on what I mean here…
 - fill-in other commands
   Not all commands are supported in the submitted patch. I think once   we have a clear documentation on the general
JSONformating and how   to use it as a user, we need to include support for all CREATE   commands that we have. 
   I see nothing against extending when this work has to bo done until   after the CF, as long as it's fully done
beforebeta. After all it's   only about filling in minutia at this point. 
 - review the JSON producing code
   It might be possible to use more of the internal supports for JSON   now that the format is freezing.
 - regression tests
   The patch will need some. The simpler solution is to add a new   regression test entry and exercise all the CREATE
commandsin there,   in a specific schema, activating an event trigger that outputs the   JSON detailed information each
time(the snitch() example). 
   Best would be to have some "pretty" indented output of the JSON to   help with reviewing diffs, and I have to wonder
aboutJSON object   inner-ordering if we're going to do that. 
   No other ideas on this topic from me.

> The JSON parsing is done in event_trigger.c.  This code should probably
> live elsewhere, but I again hesitate to put it in json.c or jsonfuncs.c,
> at least until some discussion about its general applicability takes
> place.

I see that as useful enough if it can be made to work without the
special "fmt" fields somehow, with a nice default formatting ability.

In particular, being able to build some intermediate object with
json_agg then call the formating/expanding function on top of that might
be quite useful.

That said, I don't think we have enough time to attack this problem now,
I think it would be wiser to address your immediate problem separately
in your patch and clean it later (next release) with sharing code and
infrastructure and offering a more generally useful tool. At least we
will have some feedback about the Event Trigger specific context then.

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