Re: Command Triggers, patch v11

On 24 February 2012 22:39, Thom Brown <thom(at)linux(dot)com> wrote:
> On 24 February 2012 22:32, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 24 February 2012 22:04, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)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.

--
Thom