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
