Command Triggers, patch v11

On 24 February 2012 22:04, Dimitri Fontaine <dimitri@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.

--
Thom

On 24 February 2012 22:32, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:04, Dimitri Fontaine <dimitri@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)

--
Thom

On 24 February 2012 22:39, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:32, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:04, Dimitri Fontaine <dimitri@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.

--
Thom

On 24 February 2012 23:01, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:39, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:32, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:04, Dimitri Fontaine <dimitri@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

On 24 February 2012 23:43, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 23:01, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:39, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:32, Thom Brown <thom@linux.com> wrote:

On 24 February 2012 22:04, Dimitri Fontaine <dimitri@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

Hi,

Please find attached version 12 of the patch, which is fixing docs per
your review. Thanks for your time, comments and fixes!

You can see the patch-on-patch here for quick proof reading:

https://github.com/dimitri/postgres/commit/b7798e8ba6c9bee1f65b233316ae9c08b78e5ddb

Thom Brown <thom@linux.com> writes:

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.

Those are in the attached, apart from your editing of the examples para.
A single para is needed around all examples, which was forgotten in my
previous version of the patch, now fixed.

Thom Brown <thom@linux.com> writes:

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)

Included.

Thom Brown <thom@linux.com> writes:

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.

Seems like I've forgotten to update the docs when acting on Robert's
suggestion to improve the syntax to CREATE COMMAND TRIGGER. I've now
fixed that.

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.

Fixed too.

Thom Brown <thom@linux.com> writes:

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

Any reason why? I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

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.

Yeah I did use an emacs macro to get from the gram.y format to the docs
format, then replaced '_P ' with ''. Should have replaced '_P' really,
now done.

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?

Well you can only alter a command that you were successful in creating,
right? So I'm not sure that's needed here. By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan. If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom. It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent. You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Thanks

--
Thom

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR: invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

--
Thom

On 25 February 2012 12:42, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR:  invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall. I'd
expect ALTER COMMAND TRIGGER to output too for when individual
commands are disabled etc.

--
Thom

On 25 February 2012 13:15, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:42, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR:  invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall.  I'd
expect ALTER COMMAND TRIGGER to output too for when individual
commands are disabled etc.

Just found another case where a table can be created without a command
trigger firing:

SELECT * INTO badname FROM goodname;

--
Thom

On 25 February 2012 13:28, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 13:15, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:42, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR:  invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall.  I'd
expect ALTER COMMAND TRIGGER to output too for when individual
commands are disabled etc.

Just found another case where a table can be created without a command
trigger firing:

SELECT * INTO badname FROM goodname;

Right, hopefully this should be my last piece of list spam for the
time being. (apologies, I thought I'd just try it out at first, but
it's ended up being reviewed piecemeal)

On CREATE COMMAND TRIGGER page:

“The trigger will be associated with the specified command and will
execute the specified function function_name when that command is
run.”
should be:
“The trigger will be associated with the specified commands and will
execute the specified function function_name when those commands are
run.”

“A command trigger's function must return void, the only it can aborts
the execution of the command is by raising an exception.”
should be:
“A command trigger's function must return void. It can then only
abort the execution of the command by raising an exception.”

Remove:
“For a constraint trigger, this is also the name to use when modifying
the trigger's behavior using SET CONSTRAINTS.”

Remove:
“That leaves out the following list of non supported commands.”

s/exercize/exercise/

“that's the case for VACUUM, CLUSTER CREATE INDEX CONCURRENTLY, and
REINDEX DATABASE.”
should be:
“that's the case for VACUUM, CLUSTER, CREATE INDEX CONCURRENTLY, and
REINDEX DATABASE.”

I don’t understand this sentence:
“Triggers on ANY command support more commands than just this list,
and will only provide the command tag argument as NOT NULL.”

On ALTER COMMAND TRIGGER page:

“ALTER COMMAND TRIGGER name ON command SET enabled”
should be:
“ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled”

On DROP COMMAND TRIGGER page:

There’s a mention of CASCADE and RESTRICT. I don’t know of any object
which could be dependant on a command trigger, so I don’t see what
these are for.

An oddity I’ve noticed is that you can add additional commands to an
existing command trigger, and you can also have them execute a
different function to the other commands referenced in the same
trigger.

--
Thom

On 25 February 2012 14:30, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 13:28, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 13:15, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:42, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR:  invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall.  I'd
expect ALTER COMMAND TRIGGER to output too for when individual
commands are disabled etc.

Just found another case where a table can be created without a command
trigger firing:

SELECT * INTO badname FROM goodname;

Right, hopefully this should be my last piece of list spam for the
time being. (apologies, I thought I'd just try it out at first, but
it's ended up being reviewed piecemeal)

I was wrong.. a couple of corrections to my own response:

On CREATE COMMAND TRIGGER page:

“The trigger will be associated with the specified command and will
execute the specified function function_name when that command is
run.”
should be:
“The trigger will be associated with the specified commands and will
execute the specified function function_name when those commands are
run.”

Actually, perhaps "...when any of those commands..."

On ALTER COMMAND TRIGGER page:

“ALTER COMMAND TRIGGER name ON command SET enabled”
should be:
“ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled”

This one is nonsense, so please ignore it.

--
Thom

On 25 February 2012 16:36, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 14:30, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 13:28, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 13:15, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:42, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:07, Thom Brown <thom@linux.com> wrote:

On 25 February 2012 12:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

D'oh, just as I sent some more queries...

Thom Brown <thom@linux.com> writes:

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

Any reason why?  I don't suppose it's really important one way or the
other, so I'm waiting on some more voices before working on it.

Just so it's easy to scan.  If someone is looking for CREATE CAST,
they'd kind of expect it near the drop of the CREATE list, but it's
actually toward the bottom.  It just looks random at the moment.

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?

Well you can only alter a command that you were successful in creating,
right?  So I'm not sure that's needed here.  By that count though, I
maybe should remove the supported command list from DROP COMMAND TRIGGER
reference page?

Sure, that would be more consistent.  You're right, it's not needed.
It just seemed odd that one of the statements lacked what both others
had.

Yet another comment... (I should have really started looking at this
at an earlier stage)

It seems that if one were to enforce a naming convention for relations
as shown in the 2nd example for CREATE COMMAND TRIGGER, it could be
circumvented by someone using CREATE TABLE name AS...

test=# CREATE TABLE badname (id int, a int, b text);
ERROR:  invalid relation name: badname
test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

CREATE COMMAND TRIGGER doesn't output in pg_dump or pg_dumpall.  I'd
expect ALTER COMMAND TRIGGER to output too for when individual
commands are disabled etc.

Just found another case where a table can be created without a command
trigger firing:

SELECT * INTO badname FROM goodname;

Right, hopefully this should be my last piece of list spam for the
time being. (apologies, I thought I'd just try it out at first, but
it's ended up being reviewed piecemeal)

I was wrong.. a couple of corrections to my own response:

On CREATE COMMAND TRIGGER page:

“The trigger will be associated with the specified command and will
execute the specified function function_name when that command is
run.”
should be:
“The trigger will be associated with the specified commands and will
execute the specified function function_name when those commands are
run.”

Actually, perhaps "...when any of those commands..."

On ALTER COMMAND TRIGGER page:

“ALTER COMMAND TRIGGER name ON command SET enabled”
should be:
“ALTER COMMAND TRIGGER name ON command [, ... ] SET enabled”

This one is nonsense, so please ignore it.

Further testing reveals a problem with FTS configurations when using
the example function provided in the docs:

test=# CREATE TEXT SEARCH CONFIGURATION test (
PARSER = "default"
);
ERROR: invalid relation name:
test=# CREATE TEXT SEARCH CONFIGURATION fr_test (
PARSER = "default"
);
ERROR: invalid relation name:

The 2nd one should work as it matches the naming convention checked in
the function. The ALTER and DROP equivalents appear to be fine
though.

DROP CAST shares a similar issue too:

test=# DROP CAST (bigint as int4);
ERROR: invalid relation name: �

The odd thing about this one is that CREATE CAST shouldn't match on
name at all, but it creates a cast successfully, whereas DROP CAST
disagrees with the name.

Command triggers for CREATE TYPE don't work, but fine for ALTER TYPE
and DROP TYPE.

Also command triggers for DROP CONVERSION aren't working. A glance at
pg_cmdtrigger shows that the system views the command as "DROP
CONVERSION_P".

What is DROP ASSERTION? It's showing as a valid command for a command
trigger, but it's not documented.

I've noticed that ALTER <object> name OWNER TO role doesn't result in
any trigger being fired except for tables.

ALTER OPERATOR FAMILY .... RENAME TO ... doesn't fire command triggers.

ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command
triggers, but with SET SCHEMA it does.

And there's no command trigger available for ALTER VIEW.

I'll hold off on testing any further until a new patch is available.

--
Thom

Thanks for your further testing!

Thom Brown <thom@linux.com> writes:

Further testing reveals a problem with FTS configurations when using
the example function provided in the docs:

Could you send me your tests so that I add them to the proper regression
test? I've been lazy on one or two object types and obviously that's
where I have to check some more.

Also command triggers for DROP CONVERSION aren't working. A glance at
pg_cmdtrigger shows that the system views the command as "DROP
CONVERSION_P".

That's easy to fix, that's a typo in gram.y. I'm not seeing other ones
like this though.

-			   | DROP CONVERSION_P					{ $$ = "DROP CONVERSION_P"; }
+			   | DROP CONVERSION_P					{ $$ = "DROP CONVERSION"; }

What is DROP ASSERTION? It's showing as a valid command for a command
trigger, but it's not documented.

It's a Not Implemented Feature for which we have the grammar support to
be able to fill a standard compliant checkbox, or something like that.
It could be better for me to remove explicit support for it in the
command triggers patch?

I've noticed that ALTER <object> name OWNER TO role doesn't result in
any trigger being fired except for tables.

ALTER OPERATOR FAMILY .... RENAME TO ... doesn't fire command triggers.

ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command
triggers, but with SET SCHEMA it does.

It seems I've forgotten to add some support here, that happens in
alter.c and is easy enough to check and complete, thanks for the
testing.

And there's no command trigger available for ALTER VIEW.

Will add.

I'll hold off on testing any further until a new patch is available.

That should happen soon. Ah, the joys of coding while kids are at home
thanks to school holidays. I can't count how many times I've been killed
by a captain and married to a princess while writing that patch, sorry
about those hiccups here.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 26 February 2012 14:12, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Thanks for your further testing!

Thom Brown <thom@linux.com> writes:

Further testing reveals a problem with FTS configurations when using
the example function provided in the docs:

Could you send me your tests so that I add them to the proper regression
test?  I've been lazy on one or two object types and obviously that's
where I have to check some more.

Which tests? The FTS Config test was what I posted before. I haven't
gone to any great effort to set up tests for each command. I've just
been making them up as I go along.

What is DROP ASSERTION?  It's showing as a valid command for a command
trigger, but it's not documented.

It's a Not Implemented Feature for which we have the grammar support to
be able to fill a standard compliant checkbox, or something like that.
It could be better for me to remove explicit support for it in the
command triggers patch?

Well considering there are commands that exist which we don't allow
triggers on, it seems weird to support triggers on commands which
aren't implemented. DROP ASSERTION doesn't appear anywhere else in
the documentation, so I can't think of how supporting a trigger for it
could be useful.

I've noticed that ALTER <object> name OWNER TO role doesn't result in
any trigger being fired except for tables.

ALTER OPERATOR FAMILY .... RENAME TO ... doesn't fire command triggers.

ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command
triggers, but with SET SCHEMA it does.

It seems I've forgotten to add some support here, that happens in
alter.c and is easy enough to check and complete, thanks for the
testing.

So would the fix cover many cases at once?

I'll hold off on testing any further until a new patch is available.

That should happen soon. Ah, the joys of coding while kids are at home
thanks to school holidays. I can't count how many times I've been killed
by a captain and married to a princess while writing that patch, sorry
about those hiccups here.

Being killed by a captain does make things more difficult, yes.

--
Thom

On 26 February 2012 19:49, Thom Brown <thom@linux.com> wrote:

On 26 February 2012 14:12, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Thanks for your further testing!

Thom Brown <thom@linux.com> writes:

Further testing reveals a problem with FTS configurations when using
the example function provided in the docs:

Could you send me your tests so that I add them to the proper regression
test?  I've been lazy on one or two object types and obviously that's
where I have to check some more.

Which tests?  The FTS Config test was what I posted before.  I haven't
gone to any great effort to set up tests for each command.  I've just
been making them up as I go along.

What is DROP ASSERTION?  It's showing as a valid command for a command
trigger, but it's not documented.

It's a Not Implemented Feature for which we have the grammar support to
be able to fill a standard compliant checkbox, or something like that.
It could be better for me to remove explicit support for it in the
command triggers patch?

Well considering there are commands that exist which we don't allow
triggers on, it seems weird to support triggers on commands which
aren't implemented.  DROP ASSERTION doesn't appear anywhere else in
the documentation, so I can't think of how supporting a trigger for it
could be useful.

I've noticed that ALTER <object> name OWNER TO role doesn't result in
any trigger being fired except for tables.

ALTER OPERATOR FAMILY .... RENAME TO ... doesn't fire command triggers.

ALTER OPERATOR CLASS with RENAME TO or OWNER TO doesn't fire command
triggers, but with SET SCHEMA it does.

It seems I've forgotten to add some support here, that happens in
alter.c and is easy enough to check and complete, thanks for the
testing.

So would the fix cover many cases at once?

I'll hold off on testing any further until a new patch is available.

That should happen soon. Ah, the joys of coding while kids are at home
thanks to school holidays. I can't count how many times I've been killed
by a captain and married to a princess while writing that patch, sorry
about those hiccups here.

Being killed by a captain does make things more difficult, yes.

I've got a question regarding the function signatures required for
command triggers, and apologies if it's already been discussed to
death (I didn't see all the original conversations around this).
These differ from regular trigger functions which don't require any
arguments, and instead use special variables. Why aren't we doing the
same for command triggers? So instead of having the parameters
tg_when, cmd_tag, objectid, schemaname and objectname, using pl/pgsql
as an example, we'd have the variables TG_WHEN (already exists), TG_OP
(already exists and equivalent to cmd_tag), TG_RELID (already exists,
although maybe not directly equivalent), TG_REL_SCHEMA (doesn't exist
but would replace schemaname) and TG_RELNAME (this is actually
deprecated but could be re-used for this purpose).

Advantages of implementing it like this is that there's consistency in
the trigger system, it's easier as no function parameters required,
and any future options you may wish to add won't break functions from
previous versions, meaning more room for adding stuff later on.

Disadvantages are that there's more maintenance overhead for
supporting multiple languages using special variables.

--
Thom

Thom Brown <thom@linux.com> writes:

test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

You won't believe it: CTAS is not implemented as a DDL. Andres did
some work about that and sent a patch that received positive reviews by
both Tom and Robert, once that's in I can easily add support for the
command.

Thanks Andres :)
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Thom Brown <thom@linux.com> writes:

SELECT * INTO badname FROM goodname;

Again, see Andres' patch about that.

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 27 February 2012 19:19, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Thom Brown <thom@linux.com> writes:

test=# CREATE TABLE badname AS SELECT 1::int id, 1::int a, ''::text b;
SELECT 1

This doesn't even get picked up by ANY COMMAND.

You won't believe it:  CTAS is not implemented as a DDL.  Andres did
some work about that and sent a patch that received positive reviews by
both Tom and Robert, once that's in I can easily add support for the
command.

Thanks Andres :)

I don't see it anywhere in the commitfest. Has it been properly submitted?

--
Thom