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?

Thom Brown <t...@linux.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.

> There is no support for ALTER CONVERSION.

It was missing in the grammar and docs only, added.

> WARNING:  CREATE INDEX CONCURRENTLY is not supported
> DETAIL:  The command trigger will not get fired.
>
> This should probably say that it’s not supported on AFTER command
> triggers yet rather than the general DDL itself.

Edited.

> Command triggers for AFTER creating rules don’t return OIDs.

Fixed.

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

> Command triggers for AFTER creating extensions with IF NOT EXISTS
> don’t fire, but do in the ANY COMMAND instance:

Fixed.

> Command triggers on CREATE TEXT SEARCH DICTIONARY show the name as garbage:

Fixed, test case added.

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

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

> Command triggers for AFTER CREATE VIEW don’t show the schema:

Couldn't reproduce, added test cases.

> Command triggers for BEFORE and AFTER ALTER DOMAIN show a garbage name
> and no schema when dropping a constraint:

Fixed, added test cases.

> Continuing with this same trigger, we do get a schema but a garbage
> name for OWNER TO:

Fixed, added test cases.

> When an ALTER EXTENSION fails to upgrade, the AFTER ANY COMMAND
> trigger fires, but not command triggers specifically for ALTER
> EXTENSION:
>
> Same on ALTER EXTENSION, when failing to add a member, the BEFORE ANY
> COMMAND trigger fires, but not the one specifically for ALTER
> EXTENSION:

Again, per design. Let's talk about it, it will probably need at least
documentation.

> Specific command triggers against ALTER FOREIGN TABLE (i.e. not ANY
> COMMAND) for BEFORE and AFTER aren’t working when renaming columns:
>
> Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND)
> don’t fire for any changes except renaming, changing owner or changing
> schema.  Everything else fails to trigger (cost, rows, setting
> configuration parameters, setting strict, security invoker etc.).:

I kept some TODO items as I feared I would get bored tomorrow otherwise…

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

> Specific command triggers on ALTER SEQUENCE don’t fire:
> Specific command triggers on ALTER TABLE don’t fire for renaming columns:
> Also renaming attributes doesn’t fire specific triggers:
> Specific command triggers on ALTER VIEW don’t fire for any type of change:

Kept on the TODO.

> Command triggers on ALTER TYPE when changing owner produce a garbage name:

Fixed along with the DOMAIN test case (same code).

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

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

> Command triggers on all DROP commands for TEXT SEARCH
> CONFIGURATION/DICTIONARY/PARSER/TEMPLATE show the schema name as the
> relation name:

Now that's strange and will keep me awake longer tomorrow.

> Still no command triggers firing for CREATE TABLE AS:

Yes, Andres made CTAS a utility command, he didn't add the code that
make them fire command triggers. I would expect his patch to get in
first, so I don't expect him to be adding that support, I think I will
have to add it when rebasing once his patch has landed.

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.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to