Re: Command Triggers, patch v11

On 24 February 2012 23:43, Thom Brown <thom(at)linux(dot)com> wrote:
> On 24 February 2012 23:01, Thom Brown <thom(at)linux(dot)com> wrote:
>> 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.
>
> 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