Re: Command Triggers, patch v11

> Thom Brown wrote:
> Dimitri Fontaine wrote:
>> Thom Brown writes:
>>> problem. It was the DROP COMMAND TRIGGER statement that garnered
>>> comment, as it makes more sense to drop the entire trigger than
>>> individual commands for that trigger.
>>
>> What you're saying here is that a single command could have more
>> than one command attached to it, and what I understand Tom, Robert
>> and Kevin are saying is that any given command trigger should only
>> be attached to a single command.
>
> I hadn't interpreted it that way

Nor had I.

> I'm still of the opinion that a single command trigger should be
able to attach to multiple commands, just not
> amendable once the trigger has been created.

That was my understanding of what we were discussing, too.

> So my vote was for:
>
> CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
> EXECUTE PROCEDURE function_name ()
>
> ALTER COMMAND TRIGGER name SET enabled
>
> DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]

Same here.

> My argument for this is that you may wish to implement the same
> trigger for tables, views and foreign tables rather than create 3
> separate triggers.

Right. What I thought I was agreeing with was the notion that you
should need to specify more than the trigger name to drop the
trigger. Rather like how you can create a trigger AFTER INSERT OR
UPDATE OR DELETE, but you don't need to specify all those events to
drop the trigger -- just the name will do.

-Kevin

On 3 March 2012 16:12, Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> Thom Brown  wrote:
>> Dimitri Fontaine  wrote:
>>> Thom Brown  writes:
>>>> problem. It was the DROP COMMAND TRIGGER statement that garnered
>>>> comment, as it makes more sense to drop the entire trigger than
>>>> individual commands for that trigger.
>>>
>>> What you're saying here is that a single command could have more
>>> than one command attached to it, and what I understand Tom, Robert
>>> and Kevin are saying is that any given command trigger should only
>>> be attached to a single command.
>>
>> I hadn't interpreted it that way
>
> Nor had I.
>
>> I'm still of the opinion that a single command trigger should be
> able to attach to multiple commands, just not
>> amendable once the trigger has been created.
>
> That was my understanding of what we were discussing, too.
>
>> So my vote was for:
>>
>> CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
>> EXECUTE PROCEDURE function_name ()
>>
>> ALTER COMMAND TRIGGER name SET enabled
>>
>> DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]
>
> Same here.
>
>> My argument for this is that you may wish to implement the same
>> trigger for tables, views and foreign tables rather than create 3
>> separate triggers.
>
> Right.  What I thought I was agreeing with was the notion that you
> should need to specify more than the trigger name to drop the
> trigger.  Rather like how you can create a trigger AFTER INSERT OR
> UPDATE OR DELETE, but you don't need to specify all those events to
> drop the trigger -- just the name will do.

Don't you mean "shouldn't need to specify more than the trigger name"?

--
Thom

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Right. What I thought I was agreeing with was the notion that you
> should need to specify more than the trigger name to drop the
> trigger. Rather like how you can create a trigger AFTER INSERT OR
> UPDATE OR DELETE, but you don't need to specify all those events to
> drop the trigger -- just the name will do.

The parallel between INSERT/UPDATE/DELETE and the trigger's command is
not working well enough, because in the data trigger case we're managing
a single catalog entry with a single command, and in the command trigger
case, in my model at least, we would be managing several catalog entries
per command.

To take an example:

CREATE COMMAND TRIGGER foo AFTER create table, create view;
DROP COMMAND TRIGGER foo;

The first command would create two catalog entries, and the second one
would delete the same two entries. It used to work this way in the
patch, then when merging with the new remove object infrastructure I
lost that ability. From the beginning Robert has been saying he didn't
want that behavior, and Tom is now saying the same, IIUC.

So we're back to one command, one catalog entry.

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

On 3 March 2012 19:25, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Right.  What I thought I was agreeing with was the notion that you
>> should need to specify more than the trigger name to drop the
>> trigger.  Rather like how you can create a trigger AFTER INSERT OR
>> UPDATE OR DELETE, but you don't need to specify all those events to
>> drop the trigger -- just the name will do.
>
> The parallel between INSERT/UPDATE/DELETE and the trigger's command is
> not working well enough, because in the data trigger case we're managing
> a single catalog entry with a single command, and in the command trigger
> case, in my model at least, we would be managing several catalog entries
> per command.
>
> To take an example:
>
>  CREATE COMMAND TRIGGER foo AFTER create table, create view;
>  DROP COMMAND TRIGGER foo;
>
> The first command would create two catalog entries, and the second one
> would delete the same two entries.  It used to work this way in the
> patch, then when merging with the new remove object infrastructure I
> lost that ability.  From the beginning Robert has been saying he didn't
> want that behavior, and Tom is now saying the same, IIUC.
>
> So we're back to one command, one catalog entry.

That sucks. I'm surprised there's no provision for overriding it on a
command-by-command basis.

I would suggest an array instead, but that sounds costly from a
look-up perspective.

--
Thom

On Sat, Mar 3, 2012 at 2:25 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Right.  What I thought I was agreeing with was the notion that you
>> should need to specify more than the trigger name to drop the
>> trigger.  Rather like how you can create a trigger AFTER INSERT OR
>> UPDATE OR DELETE, but you don't need to specify all those events to
>> drop the trigger -- just the name will do.
>
> The parallel between INSERT/UPDATE/DELETE and the trigger's command is
> not working well enough, because in the data trigger case we're managing
> a single catalog entry with a single command, and in the command trigger
> case, in my model at least, we would be managing several catalog entries
> per command.
>
> To take an example:
>
>  CREATE COMMAND TRIGGER foo AFTER create table, create view;
>  DROP COMMAND TRIGGER foo;
>
> The first command would create two catalog entries, and the second one
> would delete the same two entries.  It used to work this way in the
> patch, then when merging with the new remove object infrastructure I
> lost that ability.  From the beginning Robert has been saying he didn't
> want that behavior, and Tom is now saying the same, IIUC.
>
> So we're back to one command, one catalog entry.

I hadn't made the connection here until you read this, but I agree
there's a problem there. One command, one catalog entry is, I think,
pretty important. So that means that if want to support a trigger on
CREATE TABLE OR CREATE VIEW OR DROP EXTENSION, then the command names
(or integers that serve as proxies for them) need to go into an array
somewhere, and we had to look for arrays that contain the command
we're looking for, rather than just the command name. That might seem
prohibitively slow, but I bet if you put a proper cache in place it
isn't, because pg_cmdtrigger should be pretty small and not updated
very often. You can probably afford to seq-scan it and rebuild your
entire cache across all command types every time it changes in any
way.

But just supporting one command type per trigger seems fine for a
first version, too. There's nothing to prevent us from adding that
later.

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