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? -- Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
