Re: Command Triggers, patch v11

On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Please find attached v13 of the command trigger patch, fixing most of
> known items and rebased against master. Two important items remain to be
> done, but I figured I should keep you posted in the meantime.

Thanks Dimitri. I'll give it a spin this weekend.

> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> This seems over-complicated.  Triggers on tables do not have alterable
>> properties, why should command triggers?  I vote for
>>
>>       CREATE COMMAND TRIGGER name ... properties ...;
>>
>>       DROP COMMAND TRIGGER name;
>>
>> full stop.  If you want to run the same trigger function on some more
>> commands, add another trigger name.
>
> I reworked ALTER COMMAND TRIGGER and DROP COMMAND TRIGGER in the
> attached, and the catalog too so that the command trigger's name is now
> unique there.  I added separate index on the command name.

I see you now only allow a single command to be attached to a trigger
at creation time. I was actually fine with how it worked before, just
not with the DROP COMMAND. So, CREATE COMMAND TRIGGER could add a
trigger against several commands, but DROP COMMAND TRIGGER would drop
the whole lot as a single trigger rather than having the ability to
drop separate commands. This is how regular triggers work, in that
you can attach to several types of SQL statement, but you have to drop
the trigger as a whole rather than dropping individual statements.

Tom, could you clarify your suggestion for this? By "properties", do
you mean possibly several commands?

> Thom Brown <thom(at)linux(dot)com> writes:
>> Just so it's easy to scan.  If someone is looking for CREATE CAST,
>> they'd kind of expect it near the drop of the CREATE list, but it's
>> actually toward the bottom.  It just looks random at the moment.
>
> I did M-x sort-lines in the documentation.  Still have to add entries
> for the new catalog though.
>
>>> test=# CREATE TABLE badname (id int, a int, b text);
>>> ERROR:  invalid relation name: badname
>>> test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
>>> SELECT 1
>>>
>>> This doesn't even get picked up by ANY COMMAND.
>
> I think Andres should add an entry for his patch on the commitfest.  Is
> it ok?

I'll try Andres' patch this weekend while I test yours.

> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> FWIW, I agree with Thom on this.  If we do it as you suggest, I
>> confidently predict that it will be less than a year before we seriously
>> regret it.  Given all the discussion around this, it's borderline insane
>> to believe that the set of parameters to be passed to command triggers
>> is nailed down and won't need to change in the future.
>
> That too will need to wait for the next revision, it's just about
> finding enough cycles, which is definitely happening very soon.

Could you give your thoughts on the design?

--
Thom