Re: Command Triggers

On Tue, Feb 14, 2012 at 4:29 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
>>> An ack about the way it's now implemented would be awesome
> I'm still missing that, which is only fair, just a ping from me here.

I took a brief look at this just now, and in general I'm pleasantly
surprised. But, as you might imagine, I have some reservations:

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
the first version of the patch, just pick a couple of simple commands
like VACUUM and ANALYZE, and do just those. We can add on more later
easily enough once the basic patch is committed. Also, that way, if
you don't get to absolutely everything, we'll have a coherent subset
of the functionality, rather than a subset defined by "what Dimitri
got to".

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.

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. I
don't have very much confidence that that is in general a safe thing
to do; there may be code calling this code that expects that such
things will not happen. This diff hunk, for example, scares the crap
out of me:

- /* check that the locales can be loaded */
- CommandCounterIncrement();
- (void) pg_newlocale_from_collation(newoid);
+ /* before or instead of command trigger might have cancelled the comman
+ if (OidIsValid(newoid))
+ {
+ /* check that the locales can be loaded */
+ CommandCounterIncrement();
+ (void) pg_newlocale_from_collation(newoid);
+ }

I don't immediately understand why that's necessary, but I'm sure
there's a good reason, and I bet a nickel that there are other places
where similar adjustments are necessary but you haven't found them. I
think we should rip out the option to silently cancel the command. We
can always add that later, but if we add it here and in the first
commit I think we are going to be chasing bugs for months.

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

+ cmd->objectId = InvalidOid;
+ cmd->objectname = (char *)aggName;
+ cmd->schemaname = get_namespace_name(aggNamespace);

I'll grant you that most people probably do not execute enough DDL for
the cost of those extra get_namespace_name() calls to add up, but I'm
not completely sure it's a negligible overhead in general, and in any
case I think it's a safe bet that there will be continuing demand to
add more information to the set of things that are supplied to the
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,
too, which is that you're supplying to the trigger the aggregate name
*as supplied by the user* and the schema name as it exists at the time
we do the reverse lookup. That reverse lookup isn't guaranteed to
work at all, and it's definitely not guaranteed to produce an answer
that's consistent with the aggName field. Maybe there's no better
way, but it would at least be better to pass the namespace OID rather
than the name. That way, the name lookup can be deferred until we are
sure that we actually need to call something.

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.

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

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?

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.

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