Re: Command Triggers, patch v11

On Tuesday, March 13, 2012 09:07:32 PM Dimitri Fontaine wrote:
> Hi,
>
> Andres Freund <andres(at)anarazel(dot)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