Re: Add CREATE support to event triggers

Hi,

Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 the object 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 but really 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 exact same object again;

- the facilities around passing the created object details and
building a SQL command are made in such a way that it's trivially
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 publishing parts and
allowing an Event Trigger to get fired after CREATE but without the
extra detailed JSON formated information 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 features we want,
with the capability of hacking schema, data types, SQL
representation etc; and because I couldn't think of 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 JSON formating 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 before beta. 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 commands in 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 about JSON 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