Re: Command Triggers

On Wed, Feb 15, 2012 at 3:25 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
>> 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".
>
> I share that feeling here. Now, given the current scope of the patch, I
> think we can add in 9.2 all the commands that make sense to support
> (yes, including alter operator family). FWIW, I've been adding support
> for 16 forms of ALTER commands today, triple (implementation of alter
> rename, owner and namespace are separated).
>
> So while your reaction is perfectly understandable, I don't think that's
> the main thing here, you've just happened to see an intermediate state
> of things.

I'm just saying that nobody's realistically going to be able to verify
a patch of this size.  It's either going to get committed warts and
all, or it's going to not get committed.  Decomposing it into a series
of patches would make it possible to actually verify the logic.  I
guess on reflection I don't really care whether you decompose it at
this point; the parts are pretty independent and it's easy enough to
revert pieces of it.  But if I commit any of this it's certainly not
going to be the whole thing in one go.

> It's still possible to cancel a command by means of RAISE EXCEPTION in a
> BEFORE command trigger, or by means of an INSTEAD OF trigger that does
> nothing.

I think that there is no problem with cancelling a command via RAISE
EXCEPTION.  It's an established precedent that errors can be thrown
anywhere, and any code that doesn't deal with that is flat broken.
But I think letting either a BEFORE or INSTEAD trigger cancel the
command is going to break things, and shouldn't be allowed without a
lot of careful study.  So -1 from me on supporting INSTEAD triggers in
the first version of this.

>> 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,
>
> It's as cheap as scanning a catalog, as of now. I didn't install a new
> catalog cache for command triggers, as I don't think this code path is
> performance critical enough to pay back for the maintenance cost. Also
> consider that a big user of command triggers might have as much as a
> couple of triggers (BEFORE/AFTER) per command, that's about 300 of them.

Yowza.  A catalog scan is WAY more expensive than a syscache lookup.
I definitely don't think you can afford to have every command result
in an extra index probe into pg_cmdtrigger.  You definitely need some
kind of caching there.

Or at least, I think you do.  You could try pgbench -f foo.sql, where
foo.sql repeatedly creates and drops a function.  See if there's a
significant slowdown with your patch vs. HEAD.  If there is, you need
some caching.  You might actually need some whole new type of sinval
message to make this work efficiently.

>> 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.
>
> I'm confused here, because all error messages that needs to contain the
> namespace are doing exactly the same thing as I'm doing in my patch.

Hmm.  I wonder what happens if those errors fire after the schema has
been dropped?  I suppose the real answer here is probably to add
enough locking that that can't happen in the first place... so maybe
this isn't an issue for your patch to worry about.

>> 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.
>
> You mean tablespace here, I guess, what else? I don't think I've added
> other shared objects in there yet. I share your analyze btw, will
> remove support.

And databases.

>> 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?
>
> Well \dt and \dT are already taken, so I got there. Will change to
> something else, e.g. \dct would be your choice here?

Yeah, probably.

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