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 again just 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