Re: Command Triggers, patch v11

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.

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

> - ExecBeforeOrInsteadOfCommandTriggers is referenced in
> exec_command_triggers_internal comments
> - InitCommandContext comments are outdated
> - generally comments look a bit outdated

Fixed.

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

Right, done.

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

I've fixed that code.

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

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

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

User mapping and casts object names are composite, and I don't know how
to represent that in a single text structure.

> - DropPropertyStmt seems to be an unused leftover?

Seems so, cleaned out.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support