Re: Command Triggers

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Feb 17, 2012 at 10:42 AM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Done.  Of course at the time the command trigger is created you can't
>> distinguish if the CREATE INDEX command will be run CONCURRENTLY or not,
>> so I've decided to issue a WARNING about it.
>
> That seems icky. Whatever warnings need to be given should be in the
> documentation, not at runtime.

Agreed. We're still making user visible changes though, so I wanted to
defer docs editing some more. Even documented, a WARNING seems a good
idea to me, but maybe you would prefer a NOTICE?

> Another idea here would be to treat CREATE INDEX CONCURRENTLY as if it
> were a separate toplevel command, for command-trigger purposes only.
> But I'm not sure that's any better.

The patch as it stands will fire the AFTER command trigger only when not
using the CONCURRENTLY variant, which I think is enough, once documented.

> Yeah, I think that will be much more clear, and not really that much
> work for you. It will also make the reference pages simpler, I think,
> since there are significant behavioral differences between ordinary
> triggers and command triggers.

Yeah done this way, still needed an overview section in triggers.sgml I
think.

>> Both done, if you agree with using session_replication_role here.
>
> It's better than a sharp stick in the eye. I'm not convinced it's
> ideal, but I don't feel strongly enough about the issue to push on it
> for now, as long as we disallow command triggers on CREATE/ALTER/DROP
> COMMAND TRIGGER.

We simply don't support those commands as far as command triggers are
concerned, which seems to be like a sane limitation.

> I sure won't. I think ultimately you won't like it either, since the
> objectaddress infrastructure is also needed to make this work with
> extensions. And I assume you would agree with me that extensions are
> an important feature. :-)

How you'd guess about that :)

Will see about it later tonight, I'd like to keep the multiple command
drop command trigger spelling.

>> It's not really command strings but the Command Tag we've historically
>> been using up until now.  You're saying that it should remain the same
>> for users but change internally.  No strong opinion from me here, apart
>> from it being more code for doing the same thing.
>
> Well, the reason I thought it might be better is for caching purposes.
> If you have a cache of which triggers need to be run for which
> commands, an integer index into an array will be a lot faster than a
> hash table lookup. But it may bear more examination, so I don't feel
> this is a must-do at this point.

I've been trying to get a feeling of the runtime performance with
command triggers in the line you suggested, even if I'd be very
surprised that a couple of index scans are anything but noise when
completing a DDL command.

I'm having those results on my development machine:

duration: 30 s
number of transactions actually processed: 42390
tps = 1413.004051 (including connections establishing)
tps = 1413.505517 (excluding connections establishing)
statement latencies in milliseconds:
0.705843 create or replace function plus1(int) returns bigint language sql as $$ select $1::bigint + 1; $$;

I don't have the setup to compare that easily to current master's
branch, I was hoping you would run tests on your side (btw the previous
patch version is rebased against master and cleaned up, should be fine
now — oh and in context format).

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