Re: Command Triggers, patch v11

On 5 March 2012 20:42, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Thanks for the extensive testing.  I'm adding your tests to the
> regression suite, and keep wondering if you saw that lots of them were
> already covered?  Did you try make installcheck?

Yes, but I felt it better that I come up with my own separate tests.

> Thom Brown <thom(at)linux(dot)com> writes:
>> Creating a command trigger using ANY COMMAND results in oid,
>> schemaname, objectname (function parameters 4 & 5) not being set for
>> either BEFORE or AFTER.
>
> Yes, that's documented.  It could be better documented though, it seems.

Is there a reason why we can't provide the OID for ANY COMMAND... if
it's available? I'm guessing it would end up involving having to
special case for every command. :/

>> Command triggers for creating sequences don’t show the schema:
>
> Documented already, it's uneasy to get at it in the code and I figured I
> might as well drop the ball on that in the current patch's form.

Fair enough.

>> Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t
>> fire if the type isn’t created due to an error:
>
> Per design, although we might want to talk about it.  I made it so that
> specific command triggers are only fired after errors checks have been
> made.
>
> That's not the case with ANY command triggers so that you can actually
> block DDLs on your instances, as has been asked on list.

I don't have any strong feelings about it, so I'll bear it in mind for
future tests.

>> The ANY COMMAND trigger fires on creating roles, but there’s no
>> corresponding allowance to create the trigger explicitly for creating
>> roles.
>
> Roles are global objects, you don't want the behavior of role commands
> to depend on which database you happen to have been logged in when
> issuing the command.  That would call for removing the ANY command
> support for them too, but I can't seem to decide about that.
>
> Any input?

If that's your reasoning, then it would make sense to remove ANY
command support for it too.

>> There doesn’t appear to be command trigger support for ALTER LARGE OBJECT.
>
> Do we want to have some?  Those are in between data and command.

*shrug* But ANY COMMAND triggers fire for it. So I'd say either
remove support for that, or add a specific trigger.

>> Specific command triggers on DROP AGGREGATE don’t fire in the IF
>> EXISTS scenario if the target object doesn’t exist:
>
> So, do we want to run the command triggers here?  Is the IF EXISTS check
> to be considered like the other error conditions?

Maybe. If that's expected behaviour, I'll start expecting it then.

>> When adding objects to an extension, then dropping the extension with
>> a cascade, the objects are dropped with it, but triggers aren’t fired
>> to the removal of those dependant objects:
>
> Yes, that's expected and needs documenting.
>
>> Using DROP OWNED BY allows objects to be dropped without their
>> respective specific triggers firing.
>
> Expected too.
>
>> Using DROP SCHEMA … CASACDE also allows objects to be dropped without
>> their respective specific triggers firing:
>
> Again, same expectation here.

If these are all expected, does it in any way compromise the
effectiveness of DDL triggers in major use-cases?

> I'm not sending a revised patch, please use the github branch if you
> want to do some more tests already, or ask me for either a new patch
> version or a patch-on-patch, as you see fit.

Hmm... how does that work with regards to the commitfest process?

But I'll re-test when you let me know when you've committed your
remaining fixes to Github.

--
Thom