Re: Command Triggers - Mailing list pgsql-hackers

Hi,

Please find attached version 8 of the patch, which fixes most of your
complaints. Remaining tasks for me here: closing support for all
interesting commands, and improving docs with a section about command
triggers in the general trigger discussion in doc/src/sgml/trigger.sgml.

Robert Haas <robertmhaas@gmail.com> writes:
> 1. I fear that the collection of commands to which this applies is
> going to end up being kind of a random selection.  I suggest that for

I didn't change that (except for shared objects cleaning), and I still
think it's not too much work to add all objects we care offering command
triggers for in this release.

> 2. Currently, we have some objects (views) that support INSTEAD OF
> triggers and others (tables) that support BEFORE and AFTER triggers.
> I don't see any advantage in supporting both.

INSTEAD OF command triggers are now gone.

> 3. I am not at all convinced that it's safe to allow command triggers
> to silently (i.e. without throwing an error) cancel the operation.

That's gone too.

> 4. This API forces all the work of setting up the CommandContext to be
> done regardless of whether any command triggers are present:

Fixed, I've spent quite some time refining the API.

> trigger.  So I think that should be rethought.  It'd be nice to have a
> cheap way to say if (AnyCommandTriggersFor(command_tag)) { ... do
> stuff ... }, emphasis on cheap.  There's a definitional problem here,

The CommandContext initialization is now doing the index scan, then the
command context properties are only filled when we actually want to run
some user functions.  Both BEFORE and AFTER command triggers context
filling are now protected.

> 5. I'm not entirely convinced that it makes sense to support command
> triggers on commands that affect shared objects.  It seems odd that
> such triggers will fire or not fire depending on which database is
> currently selected.  I think that could lead to user confusion, too.

Agreed, they're now gone.

> 6. Why do we need all this extra
> copyfuncs/equalfuncs/outfuncs/readfuncs support?  I thought we were
> dropping passing node trees for 9.2.

I still have to clean that up, and as it's not as simple as cancelling
any change I've made in the files, allow me to still defer to another
version of the patch.

> 7. I don't have a strong feeling on what the psql command should be
> called, but \dcT seems odd.  Why one lower-case letter and one
> upper-case letter?

The psql command is now \dct.

> In general, I like the direction that this is going.  But I think it
> will greatly improve its chances of being successful and relatively
> non-buggy if we strip it down to something very simple for an initial
> commit, and then add more stuff piece by piece.

Having now removed support for the ability to cancel a command without
raising an error, and after some API cleaning, I think it's now much
easier to review the patch and grow confidence into it being sane.

Also, I've added regression tests.  They are only exercised by the
serial schedule (make installcheck) because they would randomly ruin the
output of the parallel schedule and make it impossible for pg_regress to
do its job here.

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