Re: Command Triggers, patch v11

On Tuesday, March 13, 2012 09:07:32 PM Dimitri Fontaine wrote:
> Hi,
> 
> Andres Freund <andres@anarazel.de> writes:
> > I did a short review of what I found after merging master
> 
> Thanks!
> 
> > - I still find it strange not to fire on cascading actions
> 
> We don't build statement for cascading so we don't fire command
> triggers. The user view is that there was no drop command on the sub
> objects, only on the main one.
> 
> I know it's not ideal, but that's a limit we have to bite for 9.2
> unfortunately.
Hm. Especially in partially replicated scenarios I think that will bite. But 
then: There will be a 9.3 at some point ;)

> > - I dislike the missing locking leading to strange errors uppon
> > concurrent changes. But then thats just about all the rest of commands/*
> > is handling it...
> > 
> > - I think list_command_triggers should do a
> > heap_lock_tuple(LockTupleShared)
> > 
> >  on the command trigger tuple. But then again just about nothing else
> >  does :(
> 
> As you say, about nothing else does. I think that's a work for another
> patch.
Not sure, that way the required work is getting bigger and bigger. But I can 
live with that... I think the command trigger work will make better 
concurrency safeness of DDL necessary.

> > 2. the switch to cmd->oldmctx made me very wary at first because I wasn't
> > sure its guaranteed to be non NULL
> > 
> > - why is there a special CommandTriggerContext if its not reset
> > separately? Should it be reset? I have to say that I dislike the api
> > around this.
> 
> Some call sites need to be able to call those functions a dynamic number
> of times. I could add a reset boolean parameter that would mostly be
> true in all call site and false in two of them (RemoveObjects,
> RemoveRelations), and add a new function to just reset the memory
> context then.
> Or maybe you have a better idea about the ideal API here?
I wonder if the answer is making the API more symmetric. Seems more future-
proof in combination to being cleaner.

//create a new memory context
InitCommandContext(cmd);

if(CommandFiresTriggers(cmd)){
    //still in current memory context, after all not much memory should be 
allocated here
    cmd.foo = bar;
    //switches memory context during function execution, resets it afterwards
    ExecBeforeCommandTriggers(cmd);
}

if(CommandFiresTriggers(cmd)){
    cmd.zap = blub;
    ExecAfterCommandTriggers(cmd);
}

//drop the memory context
CleanupCommandContext(cmd);

I find the changing of memory context in CommandFires[After]Trigger + switch 
back in Exec*CommandTrigger rather bad style and I don't really see the point 
anyway.

> > - an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema.
> > Probably the same problem exists elsewhere. Or is that as-designed?
> > Would be inconsistent with the way object names are handled.
> 
> I'm surprised, here's an excerpt from the added regression tests:
> 
> alter function notfun(int) set schema cmd;
> NOTICE:  snitch: BEFORE any ALTER FUNCTION
> NOTICE:  snitch: BEFORE ALTER FUNCTION public notfun
> NOTICE:  snitch: AFTER ALTER FUNCTION cmd notfun
> NOTICE:  snitch: AFTER any ALTER FUNCTION
I was not looking at ALTER FUNCTION but ALTER AGGREGATE. And I looked wrongly. 
Sorry for that.

Generally, uppon rereading, I have to say that I am not very happy with the 
decision that ANY triggers are fired from other places than the specific 
triggers. That seams to be a rather dangerous/confusing route to me. 
Especially because sometimes errors (permissions, duplicated names, etc) are 
raised differently in ANY than in specific triggers now:

postgres=# ALTER AGGREGATE bar.array_agg_union3(anyarray) SET SCHEMA public;
NOTICE:  when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>, 
objectname <NULL>
ERROR:  aggregate bar.array_agg_union3(anyarray) does not exist

postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA 
public;
NOTICE:  when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>, 
objectname <NULL>
ERROR:  function array_agg_union3(anyarray) is already in schema "public"

postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA bar;
NOTICE:  when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>, 
objectname <NULL>
NOTICE:  when BEFORE, tag ALTER AGGREGATE, objectid 16415, schemaname public, 
objectname array_agg_union3
NOTICE:  when AFTER, tag ALTER AGGREGATE, objectid 16415, schemaname bar, 
objectname array_agg_union3
NOTICE:  when AFTER, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>, 
objectname <NULL>


Andres