On 24 February 2012 23:43, Thom Brown <t...@linux.com> wrote: > On 24 February 2012 23:01, Thom Brown <t...@linux.com> wrote: >> On 24 February 2012 22:39, Thom Brown <t...@linux.com> wrote: >>> On 24 February 2012 22:32, Thom Brown <t...@linux.com> wrote: >>>> On 24 February 2012 22:04, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote: >>>>> Hi, >>>>> >>>>> Please find attached the latest version of the command triggers patch, >>>>> in context diff format, with support for 79 commands and documentation >>>>> about why only those, and with some limitations explained. >>>>> >>>>> I also cleaned up the node function support business that was still in >>>>> there from the command rewriting stuff that we dropped, and did a merge >>>>> from tonight's master branch (one of a few clean merges). >>>>> >>>>> This is now the whole of it, and I continue being available to make any >>>>> necessary change, although I expect only minor changes now. Thanks to >>>>> all reviewers and participants into the previous threads, you all have >>>>> allowed me to reach the current point by your precious advice, comments >>>>> and interest. >>>>> >>>>> The patch implements : >>>>> >>>>> - BEFORE/AFTER ANY command triggers >>>>> - BEFORE/AFTER command triggers for 79 documented commands >>>>> - regression tests exercised by the serial schedule only >>>>> - documentation updates with examples >>>>> >>>>> That means you need to `make installcheck` here. Installing the tests in >>>>> the parallel schedule does not lead to consistent output as installing a >>>>> command trigger will impact any other test using that command, and the >>>>> output becomes subject to the exact ordering of the concurrent tests. >>>>> >>>>> The only way for a BEFORE command triggers to change the command's >>>>> behaviour is by raising an exception that aborts the whole transaction. >>>>> >>>>> Command triggers are called with the following arguments: >>>>> >>>>> - the “event” (similar to TG_WHEN, either 'BEFORE' or 'AFTER') >>>>> - the command tag (the real one even when an ANY trigger is called) >>>>> - the object Id if available (e.g. NULL for a CREATE statement) >>>>> - the schema name (can be NULL) >>>>> - the object name (can be NULL) >>>>> >>>>> When the trigger's procedure we're calling is written in C, then another >>>>> argument is passed next, which is the parse tree Node * pointer. >>>>> >>>>> I've been talking with Marko Kreen about supporting ALTER TABLE and such >>>>> commands automatically in Londiste: given that patch, it requires >>>>> writing code in C that will rewrite the command string. It so happens >>>>> that I already have worked on that code, so we intend on bringing >>>>> support for ALTER TABLE and other commands in Skytools 3 for 9.2. >>>>> >>>>> I think the support code should be made into an extension that both >>>>> Skytools and Slony would be able to share. The extension code will be >>>>> able to adapt to major versions changes as they are released. Bucardo >>>>> would certainly be interested too, we could NOTIFY it from such an >>>>> extension. The design is yet to be done here, but it's clearly possible >>>>> to implement such a feature given the current patch. >>>>> >>>>> The ANY trigger support is mainly there to allow people to stop any DDL >>>>> traffic on their databases, then allowing it explicitly with an ALTER >>>>> COMMAND TRIGGER ... SET DISABLE when appropriate only. To that >>>>> end, the ANY command trigger is supporting more commands than you can >>>>> attach specific triggers too. >>>>> >>>>> It's also possible to ENABLE a command trigger on the REPLICA only >>>>> thanks to the session_replication_role GUC. Support for command >>>>> triggers on an Hot Standby node is not provided in this patch. >>>> >>>> Hi Dimitri, >>>> >>>> I just tried building the docs with your patch and it appears >>>> doc/src/sgml/ref/allfiles.sgml hasn't been updated with the necessary >>>> references for alterCommandTrigger, createCommandTrigger and >>>> dropCommandTrigger. >>>> >>>> Also in ref/alter_command_trigger.sgml, you define SQL-CREATETRIGGER. >>>> Shouldn't this be SQL-CREATECOMMANDTRIGGER? And there also appears to >>>> be orphaned text in the file too, such as "Forbids the execution of >>>> any DDL command". And there's a stray </para> on line 299. >>>> >>>> I attach updated versions of both of those files, which seems to fix >>>> all these problems. >>> >>> I've just noticed there's an issue with >>> doc/src/sgml/ref/alter_command_trigger.sgml too. It uses <indexterm >>> zone="sql-altertrigger"> which should be sql-altercommandtrigger. (as >>> attached) >> >> And upon trying to test the actual feature, it didn't work for me at >> all. I thought I had applied the patch incorrectly, but I hadn't, it >> was the documentation showing the wrong information. The CREATE >> COMMAND TRIGGER page actually just says CREATE TRIGGER.... BEFORE >> COMMAND <command>, which isn't the correct syntax. >> >> Also the examples on the page are incorrect in the same regard. When >> I tested it with the correction, I got an error saying that the >> function used had to return void, but the example uses bool. Upon >> also changing this, the example works as expected. > > Is there any reason why the list of commands that command triggers can > be used with isn't in alphabetical order? Also it appears to show > CREATE/ALTER/DROP TYPE_P, and the same for CONVERSION_P and DOMAIN_P. > I'm assuming these are typos? They also appear on DROP COMMAND > TRIGGER. > > The ALTER COMMAND TRIGGER page also doesn't show which commands it can > be used against. Perhaps, rather than repeat the list, there could be > a note to say that a list of valid commands can be found on the CREATE > COMMAND TRIGGER page?
I notice that DROP COMMAND TRIGGER requires the specification of every command it was created against in order to drop it. So if I had: CREATE COMMAND TRIGGER test_cmd_trg BEFORE CREATE SCHEMA, CREATE OPERATOR, CREATE COLLATION, CREATE CAST EXECUTE PROCEDURE my_func(); I couldn't drop it completely unless I specified all of those commands. Why? Incidentally, I've noticed the DROP COMMAND TRIGGER has a mistake in the syntax. DROP COMMAND TRIGGER [ IF EXISTS ] name ON COMMAND command [, ... ] [ CASCADE | RESTRICT ] Should be: DROP COMMAND TRIGGER [ IF EXISTS ] name ON command [, ... ] [ CASCADE | RESTRICT ] The documentation also needs to cover the pg_cmdtrigger catalogue as it's not mentioned anywhere. I'm enjoying playing with this feature though btw. :) -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
