Received: from makus.postgresql.org (makus.postgresql.org [98.129.198.125]) by mail.postgresql.org (Postfix) with ESMTP id 3F381159078 for ; Mon, 5 Mar 2012 16:42:30 -0400 (AST) Received: from acidenitrix.villemain.org ([91.121.90.165]) by makus.postgresql.org with esmtp (Exim 4.72) (envelope-from ) id 1S4ejk-0000iG-Iq for pgsql-hackers@postgresql.org; Mon, 05 Mar 2012 20:42:30 +0000 Received: from 89-158-241-83.rev.dartybox.com ([89.158.241.83] helo=DimIMac.local) by acidenitrix.villemain.org with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.69) (envelope-from ) id 1S4ejS-0001dT-Ap; Mon, 05 Mar 2012 21:42:10 +0100 Received: by DimIMac.local (Postfix, from userid 501) id 183243C5E35B; Mon, 5 Mar 2012 21:42:01 +0100 (CET) From: Dimitri Fontaine To: Thom Brown Cc: Dimitri Fontaine , pgsql-hackers@postgresql.org, Andres Freund , Tom Lane Subject: Re: Command Triggers, patch v11 Organization: 2ndQuadrant References: <201203041650.39826.andres@anarazel.de> User-Mail-Address: dimitri@2ndQuadrant.fr Date: Mon, 05 Mar 2012 21:42:00 +0100 In-Reply-To: (Thom Brown's message of "Mon, 5 Mar 2012 00:33:11 +0000") Message-ID: User-Agent: Gnus/5.110018 (No Gnus v0.18) Emacs/24.0.94 (darwin) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Pg-Spam-Score: -1.9 (-) X-Archive-Number: 201203/234 X-Sequence-Number: 204116 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 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=E2=80=99s not supported on AFTER command > triggers yet rather than the general DDL itself. Edited. > Command triggers for AFTER creating rules don=E2=80=99t return OIDs. Fixed. > Command triggers for creating sequences don=E2=80=99t 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=E2=80=99t fire, but do in the ANY COMMAND instance: Fixed. > Command triggers on CREATE TEXT SEARCH DICTIONARY show the name as garbag= e: Fixed, test case added. > Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don=E2=80= =99t > fire if the type isn=E2=80=99t 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=E2=80=99s 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=E2=80=99t 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=E2=80=99t working when renaming column= s: > > Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND) > don=E2=80=99t fire for any changes except renaming, changing owner or cha= nging > 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=E2= =80=A6 > There doesn=E2=80=99t appear to be command trigger support for ALTER LARG= E OBJECT. Do we want to have some? Those are in between data and command. > Specific command triggers on ALTER SEQUENCE don=E2=80=99t fire: > Specific command triggers on ALTER TABLE don=E2=80=99t fire for renaming = columns: > Also renaming attributes doesn=E2=80=99t fire specific triggers: > Specific command triggers on ALTER VIEW don=E2=80=99t fire for any type o= f 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=E2=80=99t fire in the IF > EXISTS scenario if the target object doesn=E2=80=99t 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=E2=80=99t f= ired > 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 =E2=80=A6 CASACDE also allows objects to be dropped wit= hout > 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