Re: Command Triggers, patch v11

Hi,

I did a short review of what I found after merging master 
(b4af1c25bbc636379efc5d2ffb9d420765705b8a) to what I currently fetched from 
your repo (d63df64580114de4d83cfe8eb45eb630724b8b6f).

- I still find it strange not to fire on cascading actions
- 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...T1:BEGIN;ALTER COMMAND TRIGGER cmd_before SET DISABLE;
T2:BEGIN;ALTER COMMAND TRIGGER cmd_before SET DISABLE;
T1:COMMIT;
T2:ERROR:  tuple concurrently updated

- I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)on the command trigger tuple. But then
againjust about nothing else does :(
 

- ExecBeforeOrInsteadOfCommandTriggers is referenced in 
exec_command_triggers_internal comments

- InitCommandContext comments are outdated

- generally comments look a bit outdated

- shouldn't the command trigger stuff for ALTER TABLE be done in inside 
AlterTable instead of utility.c?

- you have repetitions of the following pattern:
void
ExecBeforeCommandTriggers(CommandContext cmd)
{/* that will execute under command trigger memory context */if (cmd != NULL && cmd->before != NIL)
exec_command_triggers_internal(cmd,cmd->before, "BEFORE");
 
/* switch back to the command Memory Context now */MemoryContextSwitchTo(cmd->oldmctx);
}

1. Either cmd != NULL does not need to be checked or you need to check it 
before the MemoryContextSwitchTo
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.

- 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.

- what does that mean?
+       cmd.objectname = NULL;  /* composite object name */

- DropPropertyStmt seems to be an unused leftover?



Andres