Re: Command Triggers, patch v11

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.

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

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.

--
Thom

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)

--
Thom

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

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?

--
Thom

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

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(at)linux(dot)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(at)linux(dot)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(at)linux(dot)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(at)linux(dot)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(at)2ndquadrant(dot)fr> wrote:

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

> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>
> D'oh, just as I sent some more queries...
>
>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 12:07, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>
>> D'oh, just as I sent some more queries...
>>
>>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 12:42, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 25 February 2012 12:07, Thom Brown <thom(at)linux(dot)com> wrote:
>>> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>
>>> D'oh, just as I sent some more queries...
>>>
>>>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 13:15, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 25 February 2012 12:42, Thom Brown <thom(at)linux(dot)com> wrote:
>>> On 25 February 2012 12:07, Thom Brown <thom(at)linux(dot)com> wrote:
>>>> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>>
>>>> D'oh, just as I sent some more queries...
>>>>
>>>>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 13:28, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 25 February 2012 13:15, Thom Brown <thom(at)linux(dot)com> wrote:
>>> On 25 February 2012 12:42, Thom Brown <thom(at)linux(dot)com> wrote:
>>>> On 25 February 2012 12:07, Thom Brown <thom(at)linux(dot)com> wrote:
>>>>> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>>>
>>>>> D'oh, just as I sent some more queries...
>>>>>
>>>>>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 25 February 2012 14:30, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 25 February 2012 13:28, Thom Brown <thom(at)linux(dot)com> wrote:
>>> On 25 February 2012 13:15, Thom Brown <thom(at)linux(dot)com> wrote:
>>>> On 25 February 2012 12:42, Thom Brown <thom(at)linux(dot)com> wrote:
>>>>> On 25 February 2012 12:07, Thom Brown <thom(at)linux(dot)com> wrote:
>>>>>> On 25 February 2012 12:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>>>>
>>>>>> D'oh, just as I sent some more queries...
>>>>>>
>>>>>>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)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(at)2ndquadrant(dot)fr> wrote:
> Thanks for your further testing!
>
> Thom Brown <thom(at)linux(dot)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(at)linux(dot)com> wrote:
> On 26 February 2012 14:12, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Thanks for your further testing!
>>
>> Thom Brown <thom(at)linux(dot)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(at)linux(dot)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(at)linux(dot)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(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)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

Thom Brown <thom(at)linux(dot)com> writes:
> 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

Basically so that we don't have to special code support for each and
every language out there.

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

Lots of, so I've been told, enough of it for not taking this choice
seriously. I'll admit I didn't personally looked at what it would
entail implementation wise.

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

Thom Brown <thom(at)linux(dot)com> writes:
> 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?

Because I couldn't find a nice enough way to implement that given the
shared infrastructure Robert and Kaigai did put into place to handle
dropping of objects. It could be that I didn't look hard enough, I'll
be happy to get back that feature.

> Incidentally, I've noticed the DROP COMMAND TRIGGER has a mistake in the syntax.

Thanks, fix will be in the next version.

> The documentation also needs to cover the pg_cmdtrigger catalogue as
> it's not mentioned anywhere.

That too, working on it now, adding forgotten forms you reported and
more, adding regression tests, fixing weird cases, getting there :)

> I'm enjoying playing with this feature though btw. :)

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

On Monday, February 27, 2012 08:30:31 PM Thom Brown wrote:
> On 27 February 2012 19:19, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> > Thom Brown <thom(at)linux(dot)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.
I actually don't think anybody actually reviewed the patch so far. Tom and I
discussed the implementation strategy beforehand a bit though.

> > Thanks Andres :)
Youre welcome. Thanks for your awesome work that actually made it necessary ;)

> I don't see it anywhere in the commitfest. Has it been properly submitted?
I actually always viewed it as a part of the Dim's patch which is why I didn't
submit it as a separate patch. Maybe that was a mistake...

http://archives.postgresql.org/message-
id/201112112346(dot)07611(dot)andres(at)anarazel(dot)de contains the latest revision.

Andres

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Thom Brown <thom(at)linux(dot)com> writes:
>> 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

> Basically so that we don't have to special code support for each and
> every language out there.

FWIW, I agree with Thom on this. If we do it as you suggest, I
confidently predict that it will be less than a year before we seriously
regret it. Given all the discussion around this, it's borderline insane
to believe that the set of parameters to be passed to command triggers
is nailed down and won't need to change in the future.

As for special coding of support, it hardly seems onerous when every
language that has triggers at all has got some provision for the
existing trigger parameters. A bit of copying and pasting should get
the job done.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> FWIW, I agree with Thom on this. If we do it as you suggest, I
> confidently predict that it will be less than a year before we seriously
> regret it. Given all the discussion around this, it's borderline insane
> to believe that the set of parameters to be passed to command triggers
> is nailed down and won't need to change in the future.

I agree with the analysis…

> As for special coding of support, it hardly seems onerous when every
> language that has triggers at all has got some provision for the
> existing trigger parameters. A bit of copying and pasting should get
> the job done.

But had been (too easily) convinced not to take that route. You changed
my mind already, I'll see about changing the code too tomorrow (a cold
is having me out of steam for tonight).

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

On Monday, February 27, 2012 08:37:24 PM Andres Freund wrote:
> On Monday, February 27, 2012 08:30:31 PM Thom Brown wrote:
> > On 27 February 2012 19:19, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
wrote:
> > > Thom Brown <thom(at)linux(dot)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.
>
> I actually don't think anybody actually reviewed the patch so far. Tom and
> I discussed the implementation strategy beforehand a bit though.
>
> > > Thanks Andres :)
>
> Youre welcome. Thanks for your awesome work that actually made it necessary
> ;)
>
> > I don't see it anywhere in the commitfest. Has it been properly
> > submitted?
>
> I actually always viewed it as a part of the Dim's patch which is why I
> didn't submit it as a separate patch. Maybe that was a mistake...
>
> http://archives.postgresql.org/message-
> id/201112112346(dot)07611(dot)andres(at)anarazel(dot)de contains the latest revision.
>
I refreshed the patch so it works again on current HEAD. Basically some
trivial fixes and dfd26f9c5f371437f243249025863ea9911aacaa. The latter doesn't
seem necessary to me after the changes, so I simply ditched it. Am I missing
something?

I noticed no new things I dislike about the patch besides what I voiced last
time round:
> I attached the - from my side - final version of the patch. I dislike two
> things about it:
> * code duplication due to error handling. Before making the error message
> for various illegal SELECT INTOs the patch actually shrank the code size...
> If anybody has a good idea to avoid duplicating that loop around SelectStmt-
ops I would be happy.
> * new executor flags to define whether oids should be returned

It would be great if somebody could take a look.

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> I refreshed the patch so it works again on current HEAD. Basically some
> trivial fixes and dfd26f9c5f371437f243249025863ea9911aacaa. The latter doesn't
> seem necessary to me after the changes, so I simply ditched it. Am I missing
> something?

No, that was only needed because execMain.c was overriding somebody
else's tuple receiver. If you're putting the right receiver into the
QueryDesc to start with, it shouldn't be necessary.

I'm confused by this though:

> This basically includes a revert of d8fb1f9adbddd1eef123d66a89a9fc0ecd96f60b

I don't find that commit ID anywhere.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> schrieb:

>Andres Freund <andres(at)anarazel(dot)de> writes:
>> I refreshed the patch so it works again on current HEAD. Basically
>some
>> trivial fixes and dfd26f9c5f371437f243249025863ea9911aacaa. The
>latter doesn't
>> seem necessary to me after the changes, so I simply ditched it. Am I
>missing
>> something?
>
>No, that was only needed because execMain.c was overriding somebody
>else's tuple receiver. If you're putting the right receiver into the
>QueryDesc to start with, it shouldn't be necessary.
>
>I'm confused by this though:
>
>> This basically includes a revert of
>d8fb1f9adbddd1eef123d66a89a9fc0ecd96f60b
>
>I don't find that commit ID anywhere.
That should have been the aforementioned commit. Must have screwed up the copy&paste buffer. Sorry for that.

Andres

Please excuse the brevity and formatting - I am writing this on my mobile phone.

On 27 February 2012 19:37, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> 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?
>
> Because I couldn't find a nice enough way to implement that given the
> shared infrastructure Robert and Kaigai did put into place to handle
> dropping of objects.  It could be that I didn't look hard enough, I'll
> be happy to get back that feature.

Well the problem is that you can add commands to a trigger en masse,
but you can only remove them one at a time. Couldn't we at least
allow the removal of multiple commands at the same time? The docs you
wrote suggest you can do this, but you can't.

--
Thom

On 28 February 2012 11:43, Thom Brown <thom(at)linux(dot)com> wrote:
> On 27 February 2012 19:37, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Thom Brown <thom(at)linux(dot)com> writes:
>>> 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?
>>
>> Because I couldn't find a nice enough way to implement that given the
>> shared infrastructure Robert and Kaigai did put into place to handle
>> dropping of objects.  It could be that I didn't look hard enough, I'll
>> be happy to get back that feature.
>
> Well the problem is that you can add commands to a trigger en masse,
> but you can only remove them one at a time.  Couldn't we at least
> allow the removal of multiple commands at the same time?  The docs you
> wrote suggest you can do this, but you can't.

Also note that as of commit 66f0cf7da8eeaeca4b9894bfafd61789b514af4a
(Remove useless const qualifier) the patch no longer applies due to
changes to src/backend/commands/typecmds.c.

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> Well the problem is that you can add commands to a trigger en masse,
> but you can only remove them one at a time. Couldn't we at least
> allow the removal of multiple commands at the same time? The docs you
> wrote suggest you can do this, but you can't.

This seems over-complicated. Triggers on tables do not have alterable
properties, why should command triggers? I vote for

CREATE COMMAND TRIGGER name ... properties ...;

DROP COMMAND TRIGGER name;

full stop. If you want to run the same trigger function on some more
commands, add another trigger name.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> This seems over-complicated. Triggers on tables do not have
> alterable properties, why should command triggers? I vote for
>
> CREATE COMMAND TRIGGER name ... properties ...;
>
> DROP COMMAND TRIGGER name;
>
> full stop. If you want to run the same trigger function on some
> more commands, add another trigger name.

+1

-Kevin

On 28 February 2012 15:03, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> Well the problem is that you can add commands to a trigger en masse,
>> but you can only remove them one at a time.  Couldn't we at least
>> allow the removal of multiple commands at the same time?  The docs you
>> wrote suggest you can do this, but you can't.
>
> This seems over-complicated.  Triggers on tables do not have alterable
> properties, why should command triggers?  I vote for
>
>        CREATE COMMAND TRIGGER name ... properties ...;
>
>        DROP COMMAND TRIGGER name;
>
> full stop.  If you want to run the same trigger function on some more
> commands, add another trigger name.

This would make more sense, particularly since dropping a command
trigger, as it stands, doesn't necessarily drop the trigger.

--
Thom

Hi,

Please find attached v13 of the command trigger patch, fixing most of
known items and rebased against master. Two important items remain to be
done, but I figured I should keep you posted in the meantime.

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> This seems over-complicated. Triggers on tables do not have alterable
> properties, why should command triggers? I vote for
>
> CREATE COMMAND TRIGGER name ... properties ...;
>
> DROP COMMAND TRIGGER name;
>
> full stop. If you want to run the same trigger function on some more
> commands, add another trigger name.

I reworked ALTER COMMAND TRIGGER and DROP COMMAND TRIGGER in the
attached, and the catalog too so that the command trigger's name is now
unique there. I added separate index on the command name.

Thom Brown <thom(at)linux(dot)com> writes:
> 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.

I did M-x sort-lines in the documentation. Still have to add entries
for the new catalog though.

>> 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.

I think Andres should add an entry for his patch on the commitfest. Is
it ok?

> 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.

To be done in next patch's version, sorry about that.

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> FWIW, I agree with Thom on this. If we do it as you suggest, I
> confidently predict that it will be less than a year before we seriously
> regret it. Given all the discussion around this, it's borderline insane
> to believe that the set of parameters to be passed to command triggers
> is nailed down and won't need to change in the future.

That too will need to wait for the next revision, it's just about
finding enough cycles, which is definitely happening very soon.

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

On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Please find attached v13 of the command trigger patch, fixing most of
> known items and rebased against master. Two important items remain to be
> done, but I figured I should keep you posted in the meantime.

Thanks Dimitri. I'll give it a spin this weekend.

> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> This seems over-complicated.  Triggers on tables do not have alterable
>> properties, why should command triggers?  I vote for
>>
>>       CREATE COMMAND TRIGGER name ... properties ...;
>>
>>       DROP COMMAND TRIGGER name;
>>
>> full stop.  If you want to run the same trigger function on some more
>> commands, add another trigger name.
>
> I reworked ALTER COMMAND TRIGGER and DROP COMMAND TRIGGER in the
> attached, and the catalog too so that the command trigger's name is now
> unique there.  I added separate index on the command name.

I see you now only allow a single command to be attached to a trigger
at creation time. I was actually fine with how it worked before, just
not with the DROP COMMAND. So, CREATE COMMAND TRIGGER could add a
trigger against several commands, but DROP COMMAND TRIGGER would drop
the whole lot as a single trigger rather than having the ability to
drop separate commands. This is how regular triggers work, in that
you can attach to several types of SQL statement, but you have to drop
the trigger as a whole rather than dropping individual statements.

Tom, could you clarify your suggestion for this? By "properties", do
you mean possibly several commands?

> Thom Brown <thom(at)linux(dot)com> writes:
>> 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.
>
> I did M-x sort-lines in the documentation.  Still have to add entries
> for the new catalog though.
>
>>> 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.
>
> I think Andres should add an entry for his patch on the commitfest.  Is
> it ok?

I'll try Andres' patch this weekend while I test yours.

> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> FWIW, I agree with Thom on this.  If we do it as you suggest, I
>> confidently predict that it will be less than a year before we seriously
>> regret it.  Given all the discussion around this, it's borderline insane
>> to believe that the set of parameters to be passed to command triggers
>> is nailed down and won't need to change in the future.
>
> That too will need to wait for the next revision, it's just about
> finding enough cycles, which is definitely happening very soon.

Could you give your thoughts on the design?

--
Thom

On 2 March 2012 23:33, Thom Brown <thom(at)linux(dot)com> wrote:
> On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>> 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.
>>
>> I think Andres should add an entry for his patch on the commitfest.  Is
>> it ok?
>
> I'll try Andres' patch this weekend while I test yours.

Actually no matter which patch I apply first, they cause the other to
fail to apply.

--
Thom

On Tue, Feb 28, 2012 at 10:09 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This seems over-complicated.  Triggers on tables do not have
>> alterable properties, why should command triggers?  I vote for
>>
>>       CREATE COMMAND TRIGGER name ... properties ...;
>>
>>       DROP COMMAND TRIGGER name;
>>
>> full stop.  If you want to run the same trigger function on some
>> more commands, add another trigger name.
>
> +1

+1. I suggested the same thing a while back.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

"anarazel(at)anarazel(dot)de" <andres(at)anarazel(dot)de> schrieb:

>
>
>Thom Brown <thom(at)linux(dot)com> schrieb:
>
>>On 2 March 2012 23:33, Thom Brown <thom(at)linux(dot)com> wrote:
>>> On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
>>wrote:
>>>>>> 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.
>>>>
>>>> I think Andres should add an entry for his patch on the commitfest.
>> Is
>>>> it ok?
>>>
>>> I'll try Andres' patch this weekend while I test yours.
>>
>>Actually no matter which patch I apply first, they cause the other to
>>fail to apply.
>I will try to fix that on the plain tomorrow (to NYC) but I am not yet
>sure when/where I will have internet access again.
One more try: To the list.

Andres

Please excuse the brevity and formatting - I am writing this on my mobile phone.

On 3 March 2012 00:08, Thom Brown <thom(at)linux(dot)com> wrote:
> On 2 March 2012 23:33, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>>> 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.
>>>
>>> I think Andres should add an entry for his patch on the commitfest.  Is
>>> it ok?
>>
>> I'll try Andres' patch this weekend while I test yours.
>
> Actually no matter which patch I apply first, they cause the other to
> fail to apply.

And having tried building it, it appears to fail.

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -I../../../src/include
-D_GNU_SOURCE -I/usr/include/libxml2 -c -o alter.o alter.c -MMD -MP
-MF .deps/alter.Po
alter.c: In function ‘ExecRenameStmt’:
alter.c:69:4: warning: passing argument 1 of ‘RenameCmdTrigger’ from
incompatible pointer type [enabled by default]
../../../src/include/commands/cmdtrigger.h:45:13: note: expected
‘const char *’ but argument is of type ‘struct List *’
alter.c:69:4: error: too many arguments to function ‘RenameCmdTrigger’
../../../src/include/commands/cmdtrigger.h:45:13: note: declared here

You've changed the signature of RenameCmdTrigger but still pass in the
old arguments in alter.c. If I fix this locally, I then get:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -I../../../src/include
-D_GNU_SOURCE -I/usr/include/libxml2 -c -o copyfuncs.o copyfuncs.c
-MMD -MP -MF .deps/copyfuncs.Po
copyfuncs.c: In function ‘_copyAlterCmdTrigStmt’:
copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’
copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’
copyfuncs.c:3479:2: error: ‘AlterCmdTrigStmt’ has no member named ‘command’

There's an erroneous "COPY_STRING_FIELD(command)" in there, as well as
in equalfuncs.c. After removing both those instances it builds.

Are you sure you don't have any uncommitted changes?

--
Thom

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>       CREATE COMMAND TRIGGER name ... properties ...;
>>>       DROP COMMAND TRIGGER name;
>>>
>>> full stop.  If you want to run the same trigger function on some
>>> more commands, add another trigger name.
>>
>> +1
>
> +1. I suggested the same thing a while back.

Yeah, I know, I just wanted to hear from more people before ditching out
a part of the work I did, and Thom was balancing in the opposite
direction.

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

On 3 March 2012 13:45, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>       CREATE COMMAND TRIGGER name ... properties ...;
>>>>       DROP COMMAND TRIGGER name;
>>>>
>>>> full stop.  If you want to run the same trigger function on some
>>>> more commands, add another trigger name.
>>>
>>> +1
>>
>> +1.  I suggested the same thing a while back.
>
> Yeah, I know, I just wanted to hear from more people before ditching out
> a part of the work I did, and Thom was balancing in the opposite
> direction.

I was? I agreed with Tom's comment, but I did query your
interpretation of it with regards to the CREATE COMMAND TRIGGER
statement. It seems you removed the ability to create a command
trigger against multiple commands, but I don't think that was the
problem. It was the DROP COMMAND TRIGGER statement that garnered
comment, as it makes more sense to drop the entire trigger than
individual commands for that trigger. Initially I had proposed a way
to drop all commands on a trigger at once as an additional option, but
just dropping it completely or not at all is preferable.

But if there is agreement to have multiple commands on a command
trigger, I'm wondering whether we should have an OR separator rather
than a comma? The reason is that regular triggers define a list of
statements this way. Personally I prefer the comma syntax, but my
concern (not a strong concern) is for lack of consistency.

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> And having tried building it, it appears to fail.

Sorry about that, my compiler here was happy building the source (and I
had been doing make clean install along the way) and make installcheck
passed, here.

Now fixed on my github's branch, including docs.

I'll send an updated patch revision later, hopefully including pg_dump
support fixtures (well, adaptation to the new way of doing things IIUC)
and maybe with some trigger arguments rework done.

I understand that you're not blocked until that new version is out,
right? You could use either the incremental patch attached for
convenience.

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

Thom Brown <thom(at)linux(dot)com> writes:
> problem. It was the DROP COMMAND TRIGGER statement that garnered
> comment, as it makes more sense to drop the entire trigger than
> individual commands for that trigger.

What you're saying here is that a single command could have more than
one command attached to it, and what I understand Tom, Robert and Kevin
are saying is that any given command trigger should only be attached to
a single command.

If we wanted to be more consistent we would need to have a way to attach
the same trigger to both BEFORE and AFTER the command, as of now you
need two triggers here.

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

On 3 March 2012 14:26, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> And having tried building it, it appears to fail.
>
> Sorry about that, my compiler here was happy building the source (and I
> had been doing make clean install along the way) and make installcheck
> passed, here.
>
> Now fixed on my github's branch, including docs.
>
> I'll send an updated patch revision later, hopefully including pg_dump
> support fixtures (well, adaptation to the new way of doing things IIUC)
> and maybe with some trigger arguments rework done.
>
> I understand that you're not blocked until that new version is out,
> right? You could use either the incremental patch attached for
> convenience.

Thanks for the extra patch. Do you see any functional changes between
now and your next patch? If so, it doesn't make sense for me to test
anything yet.

--
Thom

On 3 March 2012 14:34, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Thom Brown <thom(at)linux(dot)com> writes:
>> problem.  It was the DROP COMMAND TRIGGER statement that garnered
>> comment, as it makes more sense to drop the entire trigger than
>> individual commands for that trigger.
>
> What you're saying here is that a single command could have more than
> one command attached to it, and what I understand Tom, Robert and Kevin
> are saying is that any given command trigger should only be attached to
> a single command.

I hadn't interpreted it that way, but then that could just be my
misinterpretation. I'm still of the opinion that a single command
trigger should be able to attach to multiple commands, just not
amendable once the trigger has been created. If that's not how others
saw it, I'm outnumbered.

So my vote was for:

CREATE COMMAND TRIGGER name { BEFORE | AFTER } command [, ... ]
EXECUTE PROCEDURE function_name ()

ALTER COMMAND TRIGGER name SET enabled

DROP COMMAND TRIGGER [ IF EXISTS ] name [ CASCADE | RESTRICT ]

The only difference shown above compared to the current implementation
in your patch is [, ... ] after command in the CREATE COMMAND TRIGGER
statement.

My argument for this is that you may wish to implement the same
trigger for tables, views and foreign tables rather than create 3
separate triggers.

> If we wanted to be more consistent we would need to have a way to attach
> the same trigger to both BEFORE and AFTER the command, as of now you
> need two triggers here.

I'm not sure I understand. Attaching a trigger to both BEFORE and
AFTER isn't possible for regular triggers so I don't see why that
would introduce consistency. Could you explain?

--
Thom

Hi,

On Saturday, March 03, 2012 01:08:33 AM Thom Brown wrote:
> On 2 March 2012 23:33, Thom Brown <thom(at)linux(dot)com> wrote:
> > On 2 March 2012 22:32, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> >>>> 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.
> >>
> >> I think Andres should add an entry for his patch on the commitfest. Is
> >> it ok?
> >
> > I'll try Andres' patch this weekend while I test yours.
>
> Actually no matter which patch I apply first, they cause the other to
> fail to apply.
Ok, I rebased my patch ontop of dim's current HEAD. There was only one trivial
conflict in tablecmds.h. I had written the patch independently of the command
triggers stuff because I though, and still do, that would make applying it
easier.

Attached are two versions of the patch, one based on command triggers and one
without. Both pass regression tests for me.

Andres

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> FWIW, I agree with Thom on this. If we do it as you suggest, I
> confidently predict that it will be less than a year before we seriously
> regret it. Given all the discussion around this, it's borderline insane
> to believe that the set of parameters to be passed to command triggers
> is nailed down and won't need to change in the future.
>
> As for special coding of support, it hardly seems onerous when every
> language that has triggers at all has got some provision for the
> existing trigger parameters. A bit of copying and pasting should get
> the job done.

So, for that to happen I need to add a new macro and use it in most call
sites of CALLED_AS_TRIGGER(fcinfo). That includes the setup switch in
src/pl/plpgsql/src/pl_comp.c doCompile() and plpgsql_call_handler() for
starters.

Let's call the macro CALLED_AS_COMMAND_TRIGGER(fcinfo), and I would
extend CommandContextData to be a Node of type CommandTriggerData, that
needs to be added to the NodeTag enum as T_CommandTriggerData.

With that in place I can stuff the data into the function's call context
(via InitFunctionCallInfoData) then edit the call handlers of included
procedure languages until they are able to init their language variables
with the info.

Then, do we want the command trigger functions to return type trigger or
another specific type? I guess we want to forbid registering any
function as a command trigger?

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

On 4 March 2012 15:50, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Ok, I rebased my patch ontop of dim's current HEAD. There was only one trivial
> conflict in tablecmds.h. I had written the patch independently of the command
> triggers stuff because I though, and still do, that would make applying it
> easier.
>
> Attached are two versions of the patch, one based on command triggers and one
> without. Both pass regression tests for me.

I have conducted testing against Dimitri’s latest patch, along with
the incremental patch to fix the build, and also Andres’ CTAS patch.
I've attached a copy of how I configured the command triggers
(command_trigger_test_setup.txt), and also the set of tests I've been
running against the changes (command_trigger_regression.sql). I've
left comments in that last file where I haven't been able to conduct a
test for particular commands.

Creating a command trigger using ANY COMMAND results in oid,
schemaname, objectname (function parameters 4 & 5) not being set for
either BEFORE or AFTER.

There is no support for ALTER CONVERSION.

When trying to create an AFTER command trigger on CREATE INDEX, I get
the warning:

WARNING: CREATE INDEX CONCURRENTLY is not supported
DETAIL: The command trigger will not get fired.

This should probably say that it’s not supported on AFTER command
triggers yet rather than the general DDL itself.

Command triggers for AFTER creating rules don’t return OIDs.

thom(at)test=# CREATE RULE "_RETURN" AS
ON SELECT TO test2
DO INSTEAD
SELECT 234 id, 'test'::text stuff;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
RULE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE RULE'
objectid=<NULL> schemaname='public' objectname='_RETURN'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE RULE'
objectid=<NULL> schemaname='public' objectname='_RETURN'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE RULE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
CREATE RULE

Command triggers for creating sequences don’t show the schema:

thom(at)test=# CREATE TEMP SEQUENCE test_seq2;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE SEQUENCE'
objectid=<NULL> schemaname='<NULL>' objectname='test_seq2'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE SEQUENCE'
objectid=25130 schemaname='<NULL>' objectname='test_seq2'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
CREATE SEQUENCE

Command triggers for AFTER creating extensions with IF NOT EXISTS
don’t fire, but do in the ANY COMMAND instance:

thom(at)test=# CREATE EXTENSION IF NOT EXISTS file_fdw;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE EXTENSION'
objectid=<NULL> schemaname='<NULL>' objectname='file_fdw'
NOTICE: extension "file_fdw" already exists, skipping
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
CREATE EXTENSION

Command triggers on CREATE TEXT SEARCH DICTIONARY show the name as garbage:

thom(at)test=# CREATE TEXT SEARCH DICTIONARY test_stem (
test(# TEMPLATE = snowball,
test(# language = 'english', stopwords = 'english'
test(# );
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE TEXT
SEARCH DICTIONARY' objectid=<NULL> schemaname='<NULL>'
objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE TEXT SEARCH
DICTIONARY' objectid=<NULL> schemaname='thom' objectname='�Ч�l '
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE TEXT SEARCH
DICTIONARY' objectid=25139 schemaname='thom' objectname='�Ч�l '
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE TEXT
SEARCH DICTIONARY' objectid=<NULL> schemaname='<NULL>'
objectname='<NULL>'
CREATE TEXT SEARCH DICTIONARY

Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t
fire if the type isn’t created due to an error:

thom(at)test=# CREATE TYPE thom.type_test AS
(a integer,
b integer,
c text);
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
TYPE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ERROR: type "type_test" already exists

The ANY COMMAND trigger fires on creating roles, but there’s no
corresponding allowance to create the trigger explicitly for creating
roles.

Command triggers for AFTER CREATE VIEW don’t show the schema:

thom(at)test=# CREATE VIEW view_test AS SELECT id, stuff FROM public.test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
VIEW' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='view_test'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE VIEW'
objectid=25155 schemaname='<NULL>' objectname='view_test'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
CREATE VIEW

Command triggers for BEFORE and AFTER ALTER DOMAIN show a garbage name
and no schema when dropping a constraint:

thom(at)test=# ALTER DOMAIN us_postal_code DROP CONSTRAINT dummy_constraint;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='ALTER DOMAIN'
objectid=25085 schemaname='<NULL>' objectname='�'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='ALTER DOMAIN'
objectid=25085 schemaname='<NULL>' objectname='�'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER DOMAIN

Continuing with this same trigger, we do get a schema but a garbage
name for OWNER TO:

thom(at)test=# ALTER DOMAIN us_postal_code OWNER TO test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='ALTER DOMAIN'
objectid=25085 schemaname='public' objectname='�'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='ALTER DOMAIN'
objectid=25085 schemaname='public' objectname='�'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER DOMAIN

When an ALTER EXTENSION fails to upgrade, the AFTER ANY COMMAND
trigger fires, but not command triggers specifically for ALTER
EXTENSION:

thom(at)test=# ALTER EXTENSION file_fdw UPDATE TO '1.0';
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='ALTER EXTENSION'
objectid=25087 schemaname='<NULL>' objectname='file_fdw'
NOTICE: version "1.0" of extension "file_fdw" is already installed
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER EXTENSION

Same on ALTER EXTENSION, when failing to add a member, the BEFORE ANY
COMMAND trigger fires, but not the one specifically for ALTER
EXTENSION:

thom(at)test=# ALTER EXTENSION file_fdw ADD COLLATION en_gb_test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ERROR: collation "en_gb_test" for encoding "UTF8" does not exist

Specific command triggers against ALTER FOREIGN TABLE (i.e. not ANY
COMMAND) for BEFORE and AFTER aren’t working when renaming columns:

thom(at)test=# ALTER FOREIGN TABLE test.dict2 RENAME COLUMN word TO words;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
TABLE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER TABLE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER TABLE

Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND)
don’t fire for any changes except renaming, changing owner or changing
schema. Everything else fails to trigger (cost, rows, setting
configuration parameters, setting strict, security invoker etc.).:

thom(at)test=# ALTER FUNCTION test.testfunc2() COST 77;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER FUNCTION
thom(at)test=# ALTER FUNCTION srf_test() ROWS 5;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER FUNCTION
thom(at)test=# ALTER FUNCTION srf_test() RESET ALL;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER FUNCTION
thom(at)test=# ALTER FUNCTION srf_test() SET work_mem TO '1MB';
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER FUNCTION

There doesn’t appear to be command trigger support for ALTER LARGE OBJECT.

Specific command triggers on ALTER SEQUENCE don’t fire:

thom(at)test=# ALTER SEQUENCE test_seq OWNER TO test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER SEQUENCE

Specific command triggers on ALTER TABLE don’t fire for renaming columns:

thom(at)test=# ALTER TABLE testnew.test9 RENAME COLUMN stuff TO things;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
TABLE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER TABLE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER TABLE

Command triggers on ALTER TYPE when changing owner produce a garbage name:

thom(at)test=# ALTER TYPE testnew.type_test OWNER TO test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER TYPE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='ALTER TYPE'
objectid=25170 schemaname='testnew' objectname='�'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='ALTER TYPE'
objectid=25170 schemaname='testnew' objectname='�'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER TYPE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER TYPE

Also renaming attributes doesn’t fire specific triggers:

thom(at)test=# ALTER TYPE public.type_test2 RENAME ATTRIBUTE a TO z;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER TYPE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER TYPE'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER TYPE

Specific command triggers on ALTER VIEW don’t fire for any type of change:

thom(at)test=# ALTER VIEW view_test OWNER TO test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER VIEW
thom(at)test=# ALTER VIEW testnew.view_test2 ALTER COLUMN id SET DEFAULT 9;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER VIEW

Specific command triggers on DROP AGGREGATE don’t fire in the IF
EXISTS scenario if the target object doesn’t exist:

thom(at)test=# DROP AGGREGATE IF EXISTS avgtest2(bigint);
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: aggregate avgtest2(pg_catalog.int8) does not exist, skipping
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP AGGREGATE

This appears to be a general thing, as the same occurs for DROP CAST
IF EXISTS and DROP COLLATION IF EXISTS. These do, however, fire if
the object does exist.

When adding objects to an extension, then dropping the extension with
a cascade, the objects are dropped with it, but triggers aren’t fired
to the removal of those dependant objects:

thom(at)test=# ALTER EXTENSION file_fdw ADD TABLE dep_test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='ALTER EXTENSION'
objectid=25207 schemaname='<NULL>' objectname='file_fdw'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='ALTER EXTENSION'
objectid=25207 schemaname='<NULL>' objectname='file_fdw'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER EXTENSION
thom(at)test=# DROP EXTENSION file_fdw CASCADE;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP EXTENSION'
objectid=25207 schemaname='<NULL>' objectname='file_fdw'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP EXTENSION'
objectid=<NULL> schemaname='<NULL>' objectname='file_fdw'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
EXTENSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP EXTENSION

Using DROP OWNED BY allows objects to be dropped without their
respective specific triggers firing.

thom(at)test=# \dt
List of relations
Schema | Name | Type | Owner
--------+-----------------+-------+-----------
thom | role_test_table | table | role_test
thom | seq_table | table | test
(2 rows)

thom(at)test=# DROP OWNED BY role_test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP OWNED'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP OWNED'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP OWNED

Using DROP SCHEMA … CASACDE also allows objects to be dropped without
their respective specific triggers firing:

thom(at)test=# DROP SCHEMA test6 CASCADE;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
SCHEMA' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP SCHEMA'
objectid=25250 schemaname='<NULL>' objectname='test6'
NOTICE: drop cascades to table test6.test
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP SCHEMA'
objectid=<NULL> schemaname='<NULL>' objectname='test6'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP SCHEMA'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP SCHEMA

Command triggers on all DROP commands for TEXT SEARCH
CONFIGURATION/DICTIONARY/PARSER/TEMPLATE show the schema name as the
relation name:

thom(at)test=# DROP TEXT SEARCH PARSER testnew.test_parser2;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP TEXT
SEARCH PARSER' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP TEXT SEARCH
PARSER' objectid=25265 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP TEXT SEARCH
PARSER' objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP TEXT
SEARCH PARSER' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP TEXT SEARCH PARSER

Still no command triggers firing for CREATE TABLE AS:

thom(at)test=# CREATE TABLE ctas_test AS SELECT 1::int id, ''::text test;
CREATE TABLE AS

Or for SELECT * INTO... :

thom(at)test=# SELECT * INTO another_test FROM ctas_test;
SELECT INTO

Regards

--
Thom

Hi,

Thanks for the extensive testing. I'm adding your tests to the
regression suite, and keep wondering if you saw that lots of them were
already covered? Did you try make installcheck?

Thom Brown <thom(at)linux(dot)com> writes:
> Creating a command trigger using ANY COMMAND results in oid,
> schemaname, objectname (function parameters 4 & 5) not being set for
> either BEFORE or AFTER.

Yes, that's documented. It could be better documented though, it seems.

> There is no support for ALTER CONVERSION.

It was missing in the grammar and docs only, added.

> WARNING: CREATE INDEX CONCURRENTLY is not supported
> DETAIL: The command trigger will not get fired.
>
> This should probably say that it’s not supported on AFTER command
> triggers yet rather than the general DDL itself.

Edited.

> Command triggers for AFTER creating rules don’t return OIDs.

Fixed.

> Command triggers for creating sequences don’t show the schema:

Documented already, it's uneasy to get at it in the code and I figured I
might as well drop the ball on that in the current patch's form.

> Command triggers for AFTER creating extensions with IF NOT EXISTS
> don’t fire, but do in the ANY COMMAND instance:

Fixed.

> Command triggers on CREATE TEXT SEARCH DICTIONARY show the name as garbage:

Fixed, test case added.

> Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t
> fire if the type isn’t created due to an error:

Per design, although we might want to talk about it. I made it so that
specific command triggers are only fired after errors checks have been
made.

That's not the case with ANY command triggers so that you can actually
block DDLs on your instances, as has been asked on list.

> The ANY COMMAND trigger fires on creating roles, but there’s no
> corresponding allowance to create the trigger explicitly for creating
> roles.

Roles are global objects, you don't want the behavior of role commands
to depend on which database you happen to have been logged in when
issuing the command. That would call for removing the ANY command
support for them too, but I can't seem to decide about that.

Any input?

> Command triggers for AFTER CREATE VIEW don’t show the schema:

Couldn't reproduce, added test cases.

> Command triggers for BEFORE and AFTER ALTER DOMAIN show a garbage name
> and no schema when dropping a constraint:

Fixed, added test cases.

> Continuing with this same trigger, we do get a schema but a garbage
> name for OWNER TO:

Fixed, added test cases.

> When an ALTER EXTENSION fails to upgrade, the AFTER ANY COMMAND
> trigger fires, but not command triggers specifically for ALTER
> EXTENSION:
>
> Same on ALTER EXTENSION, when failing to add a member, the BEFORE ANY
> COMMAND trigger fires, but not the one specifically for ALTER
> EXTENSION:

Again, per design. Let's talk about it, it will probably need at least
documentation.

> Specific command triggers against ALTER FOREIGN TABLE (i.e. not ANY
> COMMAND) for BEFORE and AFTER aren’t working when renaming columns:
>
> Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND)
> don’t fire for any changes except renaming, changing owner or changing
> schema. Everything else fails to trigger (cost, rows, setting
> configuration parameters, setting strict, security invoker etc.).:

I kept some TODO items as I feared I would get bored tomorrow otherwise…

> There doesn’t appear to be command trigger support for ALTER LARGE OBJECT.

Do we want to have some? Those are in between data and command.

> Specific command triggers on ALTER SEQUENCE don’t fire:
> Specific command triggers on ALTER TABLE don’t fire for renaming columns:
> Also renaming attributes doesn’t fire specific triggers:
> Specific command triggers on ALTER VIEW don’t fire for any type of change:

Kept on the TODO.

> Command triggers on ALTER TYPE when changing owner produce a garbage name:

Fixed along with the DOMAIN test case (same code).

> Specific command triggers on DROP AGGREGATE don’t fire in the IF
> EXISTS scenario if the target object doesn’t exist:

So, do we want to run the command triggers here? Is the IF EXISTS check
to be considered like the other error conditions?

> When adding objects to an extension, then dropping the extension with
> a cascade, the objects are dropped with it, but triggers aren’t fired
> to the removal of those dependant objects:

Yes, that's expected and needs documenting.

> Using DROP OWNED BY allows objects to be dropped without their
> respective specific triggers firing.

Expected too.

> Using DROP SCHEMA … CASACDE also allows objects to be dropped without
> their respective specific triggers firing:

Again, same expectation here.

> Command triggers on all DROP commands for TEXT SEARCH
> CONFIGURATION/DICTIONARY/PARSER/TEMPLATE show the schema name as the
> relation name:

Now that's strange and will keep me awake longer tomorrow.

> Still no command triggers firing for CREATE TABLE AS:

Yes, Andres made CTAS a utility command, he didn't add the code that
make them fire command triggers. I would expect his patch to get in
first, so I don't expect him to be adding that support, I think I will
have to add it when rebasing once his patch has landed.

I'm not sending a revised patch, please use the github branch if you
want to do some more tests already, or ask me for either a new patch
version or a patch-on-patch, as you see fit.

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

On Monday, March 05, 2012 09:42:00 PM Dimitri Fontaine wrote:
> > Still no command triggers firing for CREATE TABLE AS:
> Yes, Andres made CTAS a utility command, he didn't add the code that
> make them fire command triggers. I would expect his patch to get in
> first, so I don't expect him to be adding that support, I think I will
> have to add it when rebasing once his patch has landed.
That was my assumption as well.

Any opinions about adding the patch to the commitfest other than from dim? I
feel a bit bad adding it to the in-progress one even if its belongs there
because is a part of the command trigger stuff...

Andres

On 5 March 2012 20:42, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Thanks for the extensive testing.  I'm adding your tests to the
> regression suite, and keep wondering if you saw that lots of them were
> already covered?  Did you try make installcheck?

Yes, but I felt it better that I come up with my own separate tests.

> Thom Brown <thom(at)linux(dot)com> writes:
>> Creating a command trigger using ANY COMMAND results in oid,
>> schemaname, objectname (function parameters 4 & 5) not being set for
>> either BEFORE or AFTER.
>
> Yes, that's documented.  It could be better documented though, it seems.

Is there a reason why we can't provide the OID for ANY COMMAND... if
it's available? I'm guessing it would end up involving having to
special case for every command. :/

>> Command triggers for creating sequences don’t show the schema:
>
> Documented already, it's uneasy to get at it in the code and I figured I
> might as well drop the ball on that in the current patch's form.

Fair enough.

>> Command triggers for BEFORE CREATE TYPE (exluding ANY COMMAND) don’t
>> fire if the type isn’t created due to an error:
>
> Per design, although we might want to talk about it.  I made it so that
> specific command triggers are only fired after errors checks have been
> made.
>
> That's not the case with ANY command triggers so that you can actually
> block DDLs on your instances, as has been asked on list.

I don't have any strong feelings about it, so I'll bear it in mind for
future tests.

>> The ANY COMMAND trigger fires on creating roles, but there’s no
>> corresponding allowance to create the trigger explicitly for creating
>> roles.
>
> Roles are global objects, you don't want the behavior of role commands
> to depend on which database you happen to have been logged in when
> issuing the command.  That would call for removing the ANY command
> support for them too, but I can't seem to decide about that.
>
> Any input?

If that's your reasoning, then it would make sense to remove ANY
command support for it too.

>> There doesn’t appear to be command trigger support for ALTER LARGE OBJECT.
>
> Do we want to have some?  Those are in between data and command.

*shrug* But ANY COMMAND triggers fire for it. So I'd say either
remove support for that, or add a specific trigger.

>> Specific command triggers on DROP AGGREGATE don’t fire in the IF
>> EXISTS scenario if the target object doesn’t exist:
>
> So, do we want to run the command triggers here?  Is the IF EXISTS check
> to be considered like the other error conditions?

Maybe. If that's expected behaviour, I'll start expecting it then.

>> When adding objects to an extension, then dropping the extension with
>> a cascade, the objects are dropped with it, but triggers aren’t fired
>> to the removal of those dependant objects:
>
> Yes, that's expected and needs documenting.
>
>> Using DROP OWNED BY allows objects to be dropped without their
>> respective specific triggers firing.
>
> Expected too.
>
>> Using DROP SCHEMA … CASACDE also allows objects to be dropped without
>> their respective specific triggers firing:
>
> Again, same expectation here.

If these are all expected, does it in any way compromise the
effectiveness of DDL triggers in major use-cases?

> I'm not sending a revised patch, please use the github branch if you
> want to do some more tests already, or ask me for either a new patch
> version or a patch-on-patch, as you see fit.

Hmm... how does that work with regards to the commitfest process?

But I'll re-test when you let me know when you've committed your
remaining fixes to Github.

--
Thom

Hi,

I believed I fixed all known glitches (all were minor quibbles here and
there because of me trying to work too fast), all I know about remaining
in the TODO list is passing the arguments the classic trigger way, I'm
waiting on a "go" from Tom on the way to do it, but well, will try on my
own if that's how busy he is (I just had enough on my plate to be able
to wait for an ack from him).

Thom Brown <thom(at)linux(dot)com> writes:
>> already covered?  Did you try make installcheck?
> Yes, but I felt it better that I come up with my own separate tests.

Ok, great then :)

> Specific command triggers agains ALTER FUNCTION (i.e. not ANY COMMAND)
> don’t fire for any changes except renaming, changing owner or changing
> schema. Everything else fails to trigger (cost, rows, setting
> configuration parameters, setting strict, security invoker etc.).:

> Specific command triggers on ALTER SEQUENCE don’t fire:
> Specific command triggers on ALTER TABLE don’t fire for renaming columns:
> Also renaming attributes doesn’t fire specific triggers:
> Specific command triggers on ALTER VIEW don’t fire for any type of change:

All previous cases and those are now fixed.

> Is there a reason why we can't provide the OID for ANY COMMAND... if
> it's available? I'm guessing it would end up involving having to
> special case for every command. :/

It's not available where it's implemented, to get the OID you need a
full integration into each code path and the idea being the ANY command
trigger is still to be able to catch more DDL than we are supporting for
specific commands.

>> Roles are global objects, you don't want the behavior of role commands
>> to depend on which database you happen to have been logged in when
>> issuing the command.  That would call for removing the ANY command
>> support for them too, but I can't seem to decide about that.
>
> If that's your reasoning, then it would make sense to remove ANY
> command support for it too.

I did that in the attached, version 14 of the patch.

>>> There doesn’t appear to be command trigger support for ALTER LARGE OBJECT.
>
> *shrug* But ANY COMMAND triggers fire for it. So I'd say either
> remove support for that, or add a specific trigger.

I tried adding that, but it's implemented in catalog/ and my tries at
having the catalog include file include the commands/cmdtrigger.h file
have been failing. I don't know how to implement that, dismissed for
now.

>> [CASCADE will not run the command triggers for cascaded objects]
> If these are all expected, does it in any way compromise the
> effectiveness of DDL triggers in major use-cases?

I don't think so. When replicating the replica will certainly drop the
same set of dependent objects, for example. Auditing is another story.
Do we want to try having cascaded object support in drop commands?

As the current code is not going through standard_ProcessUtility() in
such cases, I don't think it's possible to do for 9.2.

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

On 6 March 2012 21:04, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>> [CASCADE will not run the command triggers for cascaded objects]
>> If these are all expected, does it in any way compromise the
>> effectiveness of DDL triggers in major use-cases?
>
> I don't think so.  When replicating the replica will certainly drop the
> same set of dependent objects, for example.  Auditing is another story.
> Do we want to try having cascaded object support in drop commands?

I wasn't sure if auditing was one of the rationale behind the feature
or not. If it is, it presents a major problem. How does the replica
know that the objects were dropped?

Thanks for the updated patch and the quick turnaround time. I'll give
it another review.

--
Thom

On 6 March 2012 21:18, Thom Brown <thom(at)linux(dot)com> wrote:
> On 6 March 2012 21:04, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>> [CASCADE will not run the command triggers for cascaded objects]
>>> If these are all expected, does it in any way compromise the
>>> effectiveness of DDL triggers in major use-cases?
>>
>> I don't think so.  When replicating the replica will certainly drop the
>> same set of dependent objects, for example.  Auditing is another story.
>> Do we want to try having cascaded object support in drop commands?
>
> I wasn't sure if auditing was one of the rationale behind the feature
> or not.  If it is, it presents a major problem.  How does the replica
> know that the objects were dropped?
>
> Thanks for the updated patch and the quick turnaround time.  I'll give
> it another review.

Okay, here's the update:

The message returned by creating a command trigger after create index
is still problematic:

thom(at)test=# CREATE COMMAND TRIGGER cmd_trg_after_create_index AFTER
CREATE INDEX EXECUTE PROCEDURE cmd_trg_info();
WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported
DETAIL: The command trigger will not get fired.
CREATE COMMAND TRIGGER

The detail suggests that even though the command trigger has been
requested, it won't be fired. Might I suggest:

WARNING: AFTER CREATE INDEX CONCURRENTLY triggers are not supported
DETAIL: The command trigger will not fire on concurrently-created indexes.

CREATE VIEW doesn't return schema:

thom(at)test=# CREATE VIEW view_test AS SELECT id, stuff FROM public.test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
VIEW' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='CREATE VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='view_test'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='CREATE VIEW'
objectid=16606 schemaname='<NULL>' objectname='view_test'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='CREATE VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
CREATE VIEW

No specific triggers fire when altering a conversion:

thom(at)test=# ALTER CONVERSION test9 RENAME TO test8;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
CONVERSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
CONVERSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER CONVERSION

Note: I hadn’t previously tested this, but I don’t think it was listed
as supported until now.

No specific triggers fire when altering the properties of a function:

thom(at)test=# ALTER FUNCTION test.testfunc2() COST 77;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER FUNCTION

No specific triggers fire when altering a sequence:

thom(at)test=# ALTER SEQUENCE test_seq2 OWNER TO test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER SEQUENCE

thom(at)test=# ALTER SEQUENCE test_seq2 RESTART WITH 4;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER SEQUENCE

No specific triggers when altering a view:

thom(at)test=# ALTER VIEW view_test2 SET SCHEMA testnew;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER VIEW

thom(at)test=# ALTER VIEW testnew.view_test2 ALTER COLUMN id SET DEFAULT 9;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
ALTER VIEW

The object name shown in specific triggers when dropping aggregates
shows the schema name:

thom(at)test=# DROP AGGREGATE testnew.avgtest2(bigint);
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP AGGREGATE'
objectid=16539 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP AGGREGATE'
objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP AGGREGATE

Same for collations:

thom(at)test=# DROP COLLATION testnew.en_gb_test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
COLLATION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP COLLATION'
objectid=16542 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP COLLATION'
objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
COLLATION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP COLLATION

When dropping domains, the name of the domain includes the schema name:

thom(at)test=# DROP DOMAIN testnew.us_postal_code;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP DOMAIN'
objectid=16546 schemaname='testnew'
objectname='testnew.us_postal_code'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP DOMAIN'
objectid=<NULL> schemaname='testnew'
objectname='testnew.us_postal_code'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP DOMAIN'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP DOMAIN

Dropping functions shows the object name as the schema name:

thom(at)test=# DROP FUNCTION testnew.testfunc2();
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP FUNCTION'
objectid=16557 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP FUNCTION'
objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP FUNCTION

Same with dropping operators:

thom(at)test=# DROP OPERATOR testnew.<<|| (int2, int2);
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
OPERATOR' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP OPERATOR'
objectid=16566 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP OPERATOR'
objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
OPERATOR' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
DROP OPERATOR

...and operator family:

thom(at)test=# DROP OPERATOR FAMILY testnew.test_ops2 USING btree;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
OPERATOR FAMILY' objectid=<NULL> schemaname='<NULL>'
objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='DROP OPERATOR
FAMILY' objectid=16571 schemaname='testnew' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='DROP OPERATOR
FAMILY' objectid=<NULL> schemaname='testnew' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='DROP
OPERATOR FAMILY' objectid=<NULL> schemaname='<NULL>'
objectname='<NULL>'
DROP OPERATOR FAMILY

… and the same for dropping text search
configuration/dictionary/parser/template.

I hadn't previously tested triggers against CLUSTER, REINDEX, VACUUM
and LOAD, but have tested them now.

When creating a trigger on REINDEX, I get the following message:

thom(at)test=# CREATE COMMAND TRIGGER cmd_trg_before_reindex BEFORE
REINDEX EXECUTE PROCEDURE cmd_trg_info();
WARNING: REINDEX DATABASE is not supported
DETAIL: The command trigger will not get fired.

My problem with this is the same as what I reported previously for
CREATE INDEX, in that it suggests the DDL itself isn't supported.

But more importantly:

Creating AFTER CLUSTER command triggers produce an error (as expected
since it's not supported), but AFTER REINDEX only produces a warning.
These should be the same, probably both an error.

VACUUM doesn't fire a specific command trigger:

thom(at)test=# VACUUM;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='VACUUM'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
VACUUM

REINDEX on a table seems to show no schema name but an object name for
specific triggers:

thom(at)test=# REINDEX TABLE testnew.test9;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='REINDEX'
objectid=16558 schemaname='<NULL>' objectname='testnew'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='REINDEX'
objectid=16558 schemaname='<NULL>' objectname='testnew'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
REINDEX

When REINDEXing an index rather than a table, the table's details are
shown in the trigger. Is this expected?:

thom(at)test=# REINDEX INDEX testnew.idx_test_2 ;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: Command trigger: tg_when='BEFORE' cmd_tag='REINDEX'
objectid=16565 schemaname='testnew' objectname='test9'
NOTICE: Command trigger: tg_when='AFTER' cmd_tag='REINDEX'
objectid=16565 schemaname='testnew' objectname='test9'
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
REINDEX

REINDEXing the whole database doesn't fire specific command triggers:

thom(at)test=# REINDEX DATABASE test;
NOTICE: Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
NOTICE: table "pg_catalog.pg_class" was reindexed
<snip>
NOTICE: table "information_schema.sql_features" was reindexed
NOTICE: Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
REINDEX

The same happens for the REINDEX SYSTEM syntax.

Documentation:

I previously noted text which needed correcting that doesn’t seem to
have fixed yet. The email I reported those in misleadingly began with
“Right, hopefully this should be my last piece of list spam...”. The
only one which I feel no longer needs fixing is the first thing I
reported due to the fact that command triggers can now only been
applied against one command at a time. Also disregard the comments in
that email for ALTER and DROP.

You appear to have added support for ALTER CAST, but there isn’t any such DDL.

OPERATOR CLASS and OPERATOR FAMILY are listed twice.

“Note that objects dropped by effect of DROP CASCADE will not provoque
firing a command trigger”
should probably be:
“Note that objects dropped by the effect of DROP CASCADE will not
result in a command trigger firing.”

“That also applis to other dependencies following, as in DROP OWNED BY.”
should be:
“That also applies to other dependencies following, as in DROP OWNED BY.”

Another typo (which was in the email I referred to earlier) is
s/exercize/exercise/.

“Triggers on ANY command support more commands than just this list,
and will only provide the command tag argument as NOT NULL.”
should be:
“Triggers on ANY command support more commands than just this list,
and will provide NULL values for every argument except for the
argument that determines whether the trigger was before or after the
command event, and the command tag.”

This will no doubt need changing to refer to the new trigger variables
if you introduce them.

--
Thom

On 6 March 2012 23:25, Thom Brown <thom(at)linux(dot)com> wrote:
> On 6 March 2012 21:18, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 6 March 2012 21:04, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>>>>> [CASCADE will not run the command triggers for cascaded objects]
>>>> If these are all expected, does it in any way compromise the
>>>> effectiveness of DDL triggers in major use-cases?
>>>
>>> I don't think so.  When replicating the replica will certainly drop the
>>> same set of dependent objects, for example.  Auditing is another story.
>>> Do we want to try having cascaded object support in drop commands?
>>
>> I wasn't sure if auditing was one of the rationale behind the feature
>> or not.  If it is, it presents a major problem.  How does the replica
>> know that the objects were dropped?
>>
>> Thanks for the updated patch and the quick turnaround time.  I'll give
>> it another review.
>
> Okay, here's the update:
>
> The message returned by creating a command trigger after create index
> is still problematic:
>
> thom(at)test=# CREATE COMMAND TRIGGER cmd_trg_after_create_index AFTER
> CREATE INDEX EXECUTE PROCEDURE cmd_trg_info();
> WARNING:  AFTER CREATE INDEX CONCURRENTLY triggers are not supported
> DETAIL:  The command trigger will not get fired.
> CREATE COMMAND TRIGGER
>
> The detail suggests that even though the command trigger has been
> requested, it won't be fired.  Might I suggest:
>
> WARNING:  AFTER CREATE INDEX CONCURRENTLY triggers are not supported
> DETAIL:  The command trigger will not fire on concurrently-created indexes.
>
>
>
> CREATE VIEW doesn't return schema:
>
> thom(at)test=# CREATE VIEW view_test AS SELECT id, stuff FROM public.test;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='CREATE
> VIEW' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='CREATE VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='view_test'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='CREATE VIEW'
> objectid=16606 schemaname='<NULL>' objectname='view_test'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='CREATE VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> CREATE VIEW
>
> No specific triggers fire when altering a conversion:
>
> thom(at)test=# ALTER CONVERSION test9 RENAME TO test8;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
> CONVERSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
> CONVERSION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER CONVERSION
>
> Note: I hadn’t previously tested this, but I don’t think it was listed
> as supported until now.
>
>
> No specific triggers fire when altering the properties of a function:
>
> thom(at)test=# ALTER FUNCTION test.testfunc2() COST 77;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
> FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
> FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER FUNCTION
>
>
> No specific triggers fire when altering a sequence:
>
> thom(at)test=# ALTER SEQUENCE test_seq2 OWNER TO test;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
> SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
> SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER SEQUENCE
>
> thom(at)test=# ALTER SEQUENCE test_seq2 RESTART WITH 4;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER
> SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER
> SEQUENCE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER SEQUENCE
>
>
> No specific triggers when altering a view:
>
> thom(at)test=# ALTER VIEW view_test2 SET SCHEMA testnew;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER VIEW
>
> thom(at)test=# ALTER VIEW testnew.view_test2 ALTER COLUMN id SET DEFAULT 9;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='ALTER VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='ALTER VIEW'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> ALTER VIEW
>
>
> The object name shown in specific triggers when dropping aggregates
> shows the schema name:
>
> thom(at)test=# DROP AGGREGATE testnew.avgtest2(bigint);
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP AGGREGATE'
> objectid=16539 schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP AGGREGATE'
> objectid=<NULL> schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP
> AGGREGATE' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> DROP AGGREGATE
>
> Same for collations:
>
> thom(at)test=# DROP COLLATION testnew.en_gb_test;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> COLLATION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP COLLATION'
> objectid=16542 schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP COLLATION'
> objectid=<NULL> schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP
> COLLATION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> DROP COLLATION
>
>
> When dropping domains, the name of the domain includes the schema name:
>
> thom(at)test=# DROP DOMAIN testnew.us_postal_code;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> DOMAIN' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP DOMAIN'
> objectid=16546 schemaname='testnew'
> objectname='testnew.us_postal_code'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP DOMAIN'
> objectid=<NULL> schemaname='testnew'
> objectname='testnew.us_postal_code'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP DOMAIN'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> DROP DOMAIN
>
>
> Dropping functions shows the object name as the schema name:
>
> thom(at)test=# DROP FUNCTION testnew.testfunc2();
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP FUNCTION'
> objectid=16557 schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP FUNCTION'
> objectid=<NULL> schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP
> FUNCTION' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> DROP FUNCTION
>
>
> Same with dropping operators:
>
> thom(at)test=# DROP OPERATOR testnew.<<|| (int2, int2);
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> OPERATOR' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP OPERATOR'
> objectid=16566 schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP OPERATOR'
> objectid=<NULL> schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP
> OPERATOR' objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> DROP OPERATOR
>
>
> ...and operator family:
>
> thom(at)test=# DROP OPERATOR FAMILY testnew.test_ops2 USING btree;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='DROP
> OPERATOR FAMILY' objectid=<NULL> schemaname='<NULL>'
> objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='DROP OPERATOR
> FAMILY' objectid=16571 schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='DROP OPERATOR
> FAMILY' objectid=<NULL> schemaname='testnew' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='DROP
> OPERATOR FAMILY' objectid=<NULL> schemaname='<NULL>'
> objectname='<NULL>'
> DROP OPERATOR FAMILY
>
>
> … and the same for dropping text search
> configuration/dictionary/parser/template.
>
>
> I hadn't previously tested triggers against CLUSTER, REINDEX, VACUUM
> and LOAD, but have tested them now.
>
> When creating a trigger on REINDEX, I get the following message:
>
> thom(at)test=# CREATE COMMAND TRIGGER cmd_trg_before_reindex BEFORE
> REINDEX EXECUTE PROCEDURE cmd_trg_info();
> WARNING:  REINDEX DATABASE is not supported
> DETAIL:  The command trigger will not get fired.
>
> My problem with this is the same as what I reported previously for
> CREATE INDEX, in that it suggests the DDL itself isn't supported.
>
> But more importantly:
>
> Creating AFTER CLUSTER command triggers produce an error (as expected
> since it's not supported), but AFTER REINDEX only produces a warning.
> These should be the same, probably both an error.
>
>
> VACUUM doesn't fire a specific command trigger:
>
> thom(at)test=# VACUUM;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='VACUUM'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> VACUUM
>
>
> REINDEX on a table seems to show no schema name but an object name for
> specific triggers:
>
> thom(at)test=# REINDEX TABLE testnew.test9;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='REINDEX'
> objectid=16558 schemaname='<NULL>' objectname='testnew'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='REINDEX'
> objectid=16558 schemaname='<NULL>' objectname='testnew'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> REINDEX
>
>
> When REINDEXing an index rather than a table, the table's details are
> shown in the trigger.  Is this expected?:
>
> thom(at)test=# REINDEX INDEX testnew.idx_test_2 ;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  Command trigger: tg_when='BEFORE' cmd_tag='REINDEX'
> objectid=16565 schemaname='testnew' objectname='test9'
> NOTICE:  Command trigger: tg_when='AFTER' cmd_tag='REINDEX'
> objectid=16565 schemaname='testnew' objectname='test9'
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> REINDEX
>
>
> REINDEXing the whole database doesn't fire specific command triggers:
>
> thom(at)test=# REINDEX DATABASE test;
> NOTICE:  Command trigger on any: tg_when='BEFORE' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> NOTICE:  table "pg_catalog.pg_class" was reindexed
> <snip>
> NOTICE:  table "information_schema.sql_features" was reindexed
> NOTICE:  Command trigger on any: tg_when='AFTER' cmd_tag='REINDEX'
> objectid=<NULL> schemaname='<NULL>' objectname='<NULL>'
> REINDEX
>
> The same happens for the REINDEX SYSTEM syntax.
>
>
> Documentation:
>
> I previously noted text which needed correcting that doesn’t seem to
> have fixed yet.  The email I reported those in misleadingly began with
> “Right, hopefully this should be my last piece of list spam...”.  The
> only one which I feel no longer needs fixing is the first thing I
> reported due to the fact that command triggers can now only been
> applied against one command at a time.  Also disregard the comments in
> that email for ALTER and DROP.
>
> You appear to have added support for ALTER CAST, but there isn’t any such DDL.
>
> OPERATOR CLASS and OPERATOR FAMILY are listed twice.
>
> “Note that objects dropped by effect of DROP CASCADE will not provoque
> firing a command trigger”
> should probably be:
> “Note that objects dropped by the effect of DROP CASCADE will not
> result in a command trigger firing.”
>
> “That also applis to other dependencies following, as in DROP OWNED BY.”
> should be:
> “That also applies to other dependencies following, as in DROP OWNED BY.”
>
> Another typo (which was in the email I referred to earlier) is
> s/exercize/exercise/.
>
> “Triggers on ANY command support more commands than just this list,
> and will only provide the command tag argument as NOT NULL.”
> should be:
> “Triggers on ANY command support more commands than just this list,
> and will provide NULL values for every argument except for the
> argument that determines whether the trigger was before or after the
> command event, and the command tag.”
>
> This will no doubt need changing to refer to the new trigger variables
> if you introduce them.

I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't
think any trigger should fire on a read-only standby.

--
Thom

Hi,

Thom Brown <thom(at)linux(dot)com> writes:
> The message returned by creating a command trigger after create index
> is still problematic:

Fixed. I'm attaching an incremental patch here, the github branch is
updated too.

> CREATE VIEW doesn't return schema:

Fixed, and as an added bonus I fixed the CREATE SEQUENCE oddity about
that too.

> No specific triggers fire when altering a conversion:

Couldn't reproduce, works here, added tests.

> No specific triggers fire when altering the properties of a function:

Fixed.

> No specific triggers fire when altering a sequence:

Couldn't reproduce, added tests.

> No specific triggers when altering a view:

Same again.

> The object name shown in specific triggers when dropping aggregates
> shows the schema name:
> Same for collations:
> Dropping functions shows the object name as the schema name:
> Same with dropping operators:
> ...and operator family:
> … and the same for dropping text search
> configuration/dictionary/parser/template.

Fixed in the attached (all of those where located at exactly the same
place, by the way, that's one fix).

> When dropping domains, the name of the domain includes the schema name:

I'm using format_type_be(objectId) so that int4 is integer and int8
bigint etc, but that's adding the schemaname. I didn't have time to look
for another API that wouldn't add the schemaname nor to add one myself,
will do that soon.

> I hadn't previously tested triggers against CLUSTER, REINDEX, VACUUM
> and LOAD, but have tested them now.

Cool :)

> When creating a trigger on REINDEX, I get the following message:

Fixed.

> Creating AFTER CLUSTER command triggers produce an error (as expected
> since it's not supported), but AFTER REINDEX only produces a warning.
> These should be the same, probably both an error.

Fixed.

> VACUUM doesn't fire a specific command trigger:

I though it was better this way, I changed my mind and completed the code.

> REINDEX on a table seems to show no schema name but an object name for
> specific triggers:

Still on the TODO.

> When REINDEXing an index rather than a table, the table's details are
> shown in the trigger. Is this expected?:

Yeah well. Will see about how much damage needs to be done in the
current APIs, running out of steam for tonight's batch.

> REINDEXing the whole database doesn't fire specific command triggers:

We don't want to because REINDEX DATABASE is managing transactions on
its own, same limitation as with AFTER VACUUM and all. Will have a look
at what it takes to document that properly.

> Documentation:

Fixed.

Thom Brown <thom(at)linux(dot)com> writes:
> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
> a read-only standby, the BEFORE ANY COMMAND trigger fires. I don't
> think any trigger should fire on a read-only standby.

Well I'm not sold on that myself (think pl/untrusted that would reach
out to the OS and do whatever is needed there). You can even set the
session_replication_role GUC to replica and only have the replica
command triggers fired.

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

On 8 March 2012 22:24, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:

We're getting there. :)

> Hi,
>
> Thom Brown <thom(at)linux(dot)com> writes:
>> The message returned by creating a command trigger after create index
>> is still problematic:
>
> Fixed.  I'm attaching an incremental patch here, the github branch is
> updated too.

Confirmed.

>> CREATE VIEW doesn't return schema:
>
> Fixed, and as an added bonus I fixed the CREATE SEQUENCE oddity about
> that too.

Yes, working now.

>> No specific triggers fire when altering a conversion:
>
> Couldn't reproduce, works here, added tests.

My apologies. It seems I neglected to set up a specific trigger for
it. It does indeed work.

>> No specific triggers fire when altering the properties of a function:
>
> Fixed.

Yes, tried every property available and working in every case now.

>> No specific triggers fire when altering a sequence:
>
> Couldn't reproduce, added tests.

Again, I wrongly assumed I had set up a command trigger for this. I
think it's because I based my specific triggers on what was listed on
the CREATE COMMAND TRIGGER documentation page, and those were only
recently added. This is working fine.

>> No specific triggers when altering a view:
>
> Same again.

Working correctly. (see above)

>> The object name shown in specific triggers when dropping aggregates
>> shows the schema name:
>> Same for collations:
>> Dropping functions shows the object name as the schema name:
>> Same with dropping operators:
>> ...and operator family:
>> … and the same for dropping text search
>> configuration/dictionary/parser/template.
>
> Fixed in the attached (all of those where located at exactly the same
> place, by the way, that's one fix).

CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its
name where it's declared as "USING hash". This isn't a problem with
ALTER/DROP OPERATOR CLASS. Everything else above works as expected
now.

>> When dropping domains, the name of the domain includes the schema name:
>
> I'm using format_type_be(objectId) so that int4 is integer and int8
> bigint etc, but that's adding the schemaname. I didn't have time to look
> for another API that wouldn't add the schemaname nor to add one myself,
> will do that soon.

Okay, skipping test.

>> When creating a trigger on REINDEX, I get the following message:
>
> Fixed.

Could we change this to "REINDEX DATABASE triggers are not supported"?
This way it would be consistent with the "AFTER CREATE INDEX
CONCURRENTLY" warning.

>> VACUUM doesn't fire a specific command trigger:
>
> I though it was better this way, I changed my mind and completed the code.

Yes, working now.

>> REINDEX on a table seems to show no schema name but an object name for
>> specific triggers:
>
> Still on the TODO.

Skipped.

>> When REINDEXing an index rather than a table, the table's details are
>> shown in the trigger.  Is this expected?:
>
> Yeah well.  Will see about how much damage needs to be done in the
> current APIs, running out of steam for tonight's batch.

Skipped.

>> Documentation:
>
> Fixed.

ALTER CAST is still listed and needs removing, not just from the
documentation but every place it's used your code too. I can
currently create a trigger for it, but it's impossible for it to fire
since there's no such command.

All these corrections I mentioned previously still needs to be made:

“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.”

“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.”

> Thom Brown <thom(at)linux(dot)com> writes:
>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
>> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
>> think any trigger should fire on a read-only standby.
>
> Well I'm not sold on that myself (think pl/untrusted that would reach
> out to the OS and do whatever is needed there).  You can even set the
> session_replication_role GUC to replica and only have the replica
> command triggers fired.

All other command triggers don't fire on read-only standbys, and the
inconsistency doesn't seem right. On the one hand all
CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute
<command> in a read-only transaction" error message, but triggers do
occur before utility commands, which would otherwise display the same
message, and might not display it at all if the trigger causes an
error in its function call. So it seems like they should either all
fire, or none of them should. What are you thoughts?

--
Thom

On 9 March 2012 00:28, Thom Brown <thom(at)linux(dot)com> wrote:
> On 8 March 2012 22:24, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
>
> We're getting there. :)

It was late last night and I forgot to get around to testing pg_dump,
which isn't working correctly:

--
-- Name: cmd_trg_after_alter_aggregate; Type: COMMAND TRIGGER; Schema:
-; Owner:
--

CREATE COMMAND TRIGGER cmd_trg_after_alter_aggregate AFTER"ALTER
AGGREGATE" EXECUTE PROCEDURE cmd_trg_info ();

There shouldn't be quotes around the command, and when removing them,
ensure there's a space before the command. All variations of the
ALTER statements in the dump are fine, so are CREATE statements for
ANY COMMAND command triggers.

Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just
before CREATE/ALTER/DROP TRIGGER in the documentation. This breaks
the alphabetical order and I wasn't expecting to find it there when
scanning down the page. Could we move them into an alphabetic
position?

--
Thom

On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown <thom(at)linux(dot)com> wrote:
> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
> think any trigger should fire on a read-only standby.

Why ever not?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 March 2012 14:09, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
>> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
>> think any trigger should fire on a read-only standby.
>
> Why ever not?

Sorry, I meant any command trigger. It's because none of the commands
can be run on a standby, so the triggers don't seem appropriate.

--
Thom

On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 9 March 2012 14:09, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
>>> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
>>> think any trigger should fire on a read-only standby.
>>
>> Why ever not?
>
> Sorry, I meant any command trigger.  It's because none of the commands
> can be run on a standby, so the triggers don't seem appropriate.

I'm not convinced. Right now, it's fairly useless - all the triggers
could possibly do is throw an error, and an error is going to get
thrown anyway, so it's only a question of which error message the user
will see. But we discussed before the idea of adding a capability for
BEFORE triggers to request that the actual execution of the command
get skipped, and then it's possible to imagine this being useful.
Someone could even use a command trigger that detects which machine
it's running on, and if it's the standby, uses dblink to execute the
command on the master, or something crazy like that. Command triggers
could also be useful for logging all attempts to execute a particular
command, which is probably still appropriate on the standby.

I think that it will be a good thing to try to treat Hot Standby mode
as much like regular operation as is reasonably possible, across the
board.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 March 2012 14:30, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 9 March 2012 14:09, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Mar 7, 2012 at 4:53 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>>>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
>>>> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
>>>> think any trigger should fire on a read-only standby.
>>>
>>> Why ever not?
>>
>> Sorry, I meant any command trigger.  It's because none of the commands
>> can be run on a standby, so the triggers don't seem appropriate.
>
> I'm not convinced.  Right now, it's fairly useless - all the triggers
> could possibly do is throw an error, and an error is going to get
> thrown anyway, so it's only a question of which error message the user
> will see.  But we discussed before the idea of adding a capability for
> BEFORE triggers to request that the actual execution of the command
> get skipped, and then it's possible to imagine this being useful.
> Someone could even use a command trigger that detects which machine
> it's running on, and if it's the standby, uses dblink to execute the
> command on the master, or something crazy like that.  Command triggers
> could also be useful for logging all attempts to execute a particular
> command, which is probably still appropriate on the standby.
>
> I think that it will be a good thing to try to treat Hot Standby mode
> as much like regular operation as is reasonably possible, across the
> board.

I see your point. My suggestion to Dimitri in another email was
either enable triggers for all commands or none. At the moment it's
only available on utility commands.

--
Thom

On Fri, Mar 9, 2012 at 9:35 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> I see your point.  My suggestion to Dimitri in another email was
> either enable triggers for all commands or none.  At the moment it's
> only available on utility commands.

Yeah, that's clearly not the best of all possible worlds. :-)

I think we had better look seriously at postponing this patch to 9.3.
Your reviewing is obviously moving things forward rapidly, but I think
it's unrealistic to think this is going to be in a committable state
any time in the next week or two.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 March 2012 14:47, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 9, 2012 at 9:35 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>> I see your point.  My suggestion to Dimitri in another email was
>> either enable triggers for all commands or none.  At the moment it's
>> only available on utility commands.
>
> Yeah, that's clearly not the best of all possible worlds.  :-)
>
> I think we had better look seriously at postponing this patch to 9.3.
> Your reviewing is obviously moving things forward rapidly, but I think
> it's unrealistic to think this is going to be in a committable state
> any time in the next week or two.

That's unfortunate if that's the case. I'll dedicate any bandwidth
necessary for additional testing as I would really like to see this
get in, but if it transpires there's more outstanding work and
polishing needed than time Dimitri personally has available, then I
guess it'll have to be a 9.3 feature. :'(

--
Thom

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>> Sorry, I meant any command trigger. It's because none of the commands
>> can be run on a standby, so the triggers don't seem appropriate.

> I'm not convinced. Right now, it's fairly useless - all the triggers
> could possibly do is throw an error, and an error is going to get
> thrown anyway, so it's only a question of which error message the user
> will see. But we discussed before the idea of adding a capability for
> BEFORE triggers to request that the actual execution of the command
> get skipped, and then it's possible to imagine this being useful.

Um, surely the "you can't do that in a read-only session" error is going
to get thrown long before the command trigger could be called?

regards, tom lane

On 9 March 2012 15:05, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>>> Sorry, I meant any command trigger.  It's because none of the commands
>>> can be run on a standby, so the triggers don't seem appropriate.
>
>> I'm not convinced.  Right now, it's fairly useless - all the triggers
>> could possibly do is throw an error, and an error is going to get
>> thrown anyway, so it's only a question of which error message the user
>> will see.  But we discussed before the idea of adding a capability for
>> BEFORE triggers to request that the actual execution of the command
>> get skipped, and then it's possible to imagine this being useful.
>
> Um, surely the "you can't do that in a read-only session" error is going
> to get thrown long before the command trigger could be called?

Yes, at the moment that's the case. I said that this wasn't the case
for utility commands but I've noticed the message is different for
those:

ERROR: cannot execute VACUUM during recovery

vs

ERROR: cannot execute CREATE TABLE in a read-only transaction

So my complaint around that was misleading and wrong.

--
Thom

On Fri, Mar 9, 2012 at 10:05 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Mar 9, 2012 at 9:22 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>>> Sorry, I meant any command trigger.  It's because none of the commands
>>> can be run on a standby, so the triggers don't seem appropriate.
>
>> I'm not convinced.  Right now, it's fairly useless - all the triggers
>> could possibly do is throw an error, and an error is going to get
>> thrown anyway, so it's only a question of which error message the user
>> will see.  But we discussed before the idea of adding a capability for
>> BEFORE triggers to request that the actual execution of the command
>> get skipped, and then it's possible to imagine this being useful.
>
> Um, surely the "you can't do that in a read-only session" error is going
> to get thrown long before the command trigger could be called?

Hmmm.... yeah.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm not convinced. Right now, it's fairly useless - all the triggers
> could possibly do is throw an error, and an error is going to get
> thrown anyway, so it's only a question of which error message the user
> will see. But we discussed before the idea of adding a capability for
> BEFORE triggers to request that the actual execution of the command
> get skipped, and then it's possible to imagine this being useful.
> Someone could even use a command trigger that detects which machine
> it's running on, and if it's the standby, uses dblink to execute the
> command on the master, or something crazy like that. Command triggers
> could also be useful for logging all attempts to execute a particular
> command, which is probably still appropriate on the standby.

There are some other use cases, like using plsh to go apt-get install an
extension's package when you see the master just created it, so that
your read only queries on the hot standby have a chance of loading the
code you need.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think we had better look seriously at postponing this patch to 9.3.

I understand why you're drawing that conclusion, but I don't think
that's the best we can do here, by a long shot.

> Your reviewing is obviously moving things forward rapidly, but I think
> it's unrealistic to think this is going to be in a committable state
> any time in the next week or two.

What's happening is that I've been abusing Thom's availability, leaving
him with the testing and fixing oddities along the way. Those came
mainly from an attempt at being as automatic as possible when writing
commands support. I'm now about done reviewing each and every call site
and having them covered in the tests.

What remains to be done now is how to pass down arguments to the
triggers (switching from function arguments to trigger style magic
variables, per Tom request), and review REINDEX and CREATE OPERATOR
CLASS support. That's about it.

The API and the call sites location have been stable for a long time
now, and are following your previous round of review. The catalog
storage and commands grammar are ok too, we've been hashing them out.
We've been very careful about not introducing code path hazards, the
only novelty being a new place to ERROR out (no fancy silent utility
command execution control).

Really, I would think we're about there now. I would be rather surprised
not to be able to finish that patch by the end of next week, and will
arrange myself to be able to devote more time on it each day if that's
what needed.

Remember that we intend to build an extension providing a C-coded
function doing the heavy lifting of back parsing the command string from
the Node parse tree, with the goal of having Slony, Londiste and Bucardo
able to use that and implement support for DLLs. I really want that to
happen in 2012.

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

On Fri, Mar 9, 2012 at 12:51 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think we had better look seriously at postponing this patch to 9.3.
>
> I understand why you're drawing that conclusion, but I don't think
> that's the best we can do here, by a long shot.

Well, if you get to the point where you're done churning the code in
the next week or so, I'm willing to do one or two more rounds of
serious review, but if that doesn't get us there then I think we need
to give up. The energy you've put into this is commendable, but we're
about to start the third month of this CommitFest, and until we get
this release at least to beta or so, we can't start any new
CommitFests or branch the tree. That basically means that nothing
else of mine is going to get committed until the current crop of
patches are dealt with - or for a good while after, for that matter,
but getting the current crop of patches dealt with is the first step.
Of course, I also want to have a good release and I understand the
necessity of spending time on other people's patches as well as my
own, as I believe I've demonstrated, but I don't want to stay in that
mode indefinitely, which I think is an understandable position.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

Please find attached v15 of the patch, addressing all known issues apart
from the trigger function argument passing style. Expect a new patch
with that taken care of early next week.

(The github branch too, should you be using that)

Thom Brown <thom(at)linux(dot)com> writes:
> CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its
> name where it's declared as "USING hash". This isn't a problem with
> ALTER/DROP OPERATOR CLASS. Everything else above works as expected
> now.

Ah yes that needed a special case, it's properly handled now, and
tested.

>>> When dropping domains, the name of the domain includes the schema name:

Fixed.

> Could we change this to "REINDEX DATABASE triggers are not supported"?
> This way it would be consistent with the "AFTER CREATE INDEX
> CONCURRENTLY" warning.

Sure, done.

>>> REINDEX on a table seems to show no schema name but an object name for
>>> specific triggers:

Was a typo, fixed.

>>> When REINDEXing an index rather than a table, the table's details are
>>> shown in the trigger.  Is this expected?:

Fixed.

> ALTER CAST is still listed and needs removing, not just from the
> documentation but every place it's used your code too. I can
> currently create a trigger for it, but it's impossible for it to fire
> since there's no such command.

Removed.

> All these corrections I mentioned previously still needs to be made:

That's about the docs, I edited them accordingly to your comments.

>> Thom Brown <thom(at)linux(dot)com> writes:
> All other command triggers don't fire on read-only standbys, and the
> inconsistency doesn't seem right. On the one hand all
> CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute
> <command> in a read-only transaction" error message, but triggers do
> occur before utility commands, which would otherwise display the same
> message, and might not display it at all if the trigger causes an
> error in its function call. So it seems like they should either all
> fire, or none of them should. What are you thoughts?

The others trigger don't fire because an ERROR case is detected before
they have a chance to run, much like on a primary in some ERROR cases.

Thom Brown <thom(at)linux(dot)com> writes:
> It was late last night and I forgot to get around to testing pg_dump,
> which isn't working correctly:

Fixed.

> Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just
> before CREATE/ALTER/DROP TRIGGER in the documentation. This breaks
> the alphabetical order and I wasn't expecting to find it there when
> scanning down the page. Could we move them into an alphabetic
> position?

I don't see that problem in the source files, could you be more specific?

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

On 9 March 2012 21:38, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Please find attached v15 of the patch, addressing all known issues apart
> from the trigger function argument passing style. Expect a new patch
> with that taken care of early next week.
>
>  (The github branch too, should you be using that)
>
> Thom Brown <thom(at)linux(dot)com> writes:
>> CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its
>> name where it's declared as "USING hash".  This isn't a problem with
>> ALTER/DROP OPERATOR CLASS.  Everything else above works as expected
>> now.
>
> Ah yes that needed a special case, it's properly handled now, and
> tested.

Confirmed.

>>>> When dropping domains, the name of the domain includes the schema name:
>
> Fixed.

Confirmed.

>> Could we change this to "REINDEX DATABASE triggers are not supported"?
>>  This way it would be consistent with the "AFTER CREATE INDEX
>> CONCURRENTLY" warning.
>
> Sure, done.

Confirmed, thanks.

>>>> REINDEX on a table seems to show no schema name but an object name for
>>>> specific triggers:
>
> Was a typo, fixed.

Confirmed

>>>> When REINDEXing an index rather than a table, the table's details are
>>>> shown in the trigger.  Is this expected?:
>
> Fixed.

Confirmed.

>> ALTER CAST is still listed and needs removing, not just from the
>> documentation but every place it's used your code too.  I can
>> currently create a trigger for it, but it's impossible for it to fire
>> since there's no such command.
>
> Removed.

Confirmed that it's removed from the code. Just needs removing from
the docs too now. (doc/src/sgml/ref/create_command_trigger.sgml)

>> All these corrections I mentioned previously still needs to be made:
>
> That's about the docs, I edited them accordingly to your comments.

Confirmed.

>>> Thom Brown <thom(at)linux(dot)com> writes:
>> All other command triggers don't fire on read-only standbys, and the
>> inconsistency doesn't seem right.  On the one hand all
>> CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute
>> <command> in a read-only transaction" error message, but triggers do
>> occur before utility commands, which would otherwise display the same
>> message, and might not display it at all if the trigger causes an
>> error in its function call.  So it seems like they should either all
>> fire, or none of them should.  What are you thoughts?
>
> The others trigger don't fire because an ERROR case is detected before
> they have a chance to run, much like on a primary in some ERROR cases.

Yes, I've since discovered that I shouldn't have been treating them
equally due to the different type of error those sets of commands
generate. That's fine then.

> Thom Brown <thom(at)linux(dot)com> writes:
>> It was late last night and I forgot to get around to testing pg_dump,
>> which isn't working correctly:
>
> Fixed.

Confirmed.

>> Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just
>> before CREATE/ALTER/DROP TRIGGER in the documentation.  This breaks
>> the alphabetical order and I wasn't expecting to find it there when
>> scanning down the page.  Could we move them into an alphabetic
>> position?
>
> I don't see that problem in the source files, could you be more specific?

I can't see the problem in the source either, but when viewing
postgresql/doc/src/sgml/html/reference.html in my browser, CREATE
COMMAND TRIGGER appears between CREATE TRIGGER and CREATE TYPE. Not
sure why though.

Unless you're cleverly distracted me away from some issue that's yet
to be resolved, that appears to be nearly all my concerns addressed.
All that appears to remain is the trigger-variable stuff which you
said shall arrive early next week, and also the that odd doc issue I
mentioned in the paragraph above (although that could just be
something weird happening when I build it). Sounds like I have the
weekend off. :)

Thanks Dimitri.

--
Thom

Hi,

I did a short review of what I found after merging master
(b4af1c25bbc636379efc5d2ffb9d420765705b8a) to what I currently fetched from
your repo (d63df64580114de4d83cfe8eb45eb630724b8b6f).

- I still find it strange not to fire on cascading actions
- I dislike the missing locking leading to strange errors uppon concurrent
changes. But then thats just about all the rest of commands/* is handling
it...
T1:
BEGIN;
ALTER COMMAND TRIGGER cmd_before SET DISABLE;

T2:
BEGIN;
ALTER COMMAND TRIGGER cmd_before SET DISABLE;

T1:
COMMIT;

T2:
ERROR: tuple concurrently updated

- I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)
on the command trigger tuple. But then again just about nothing else does :(

- ExecBeforeOrInsteadOfCommandTriggers is referenced in
exec_command_triggers_internal comments

- InitCommandContext comments are outdated

- generally comments look a bit outdated

- shouldn't the command trigger stuff for ALTER TABLE be done in inside
AlterTable instead of utility.c?

- you have repetitions of the following pattern:
void
ExecBeforeCommandTriggers(CommandContext cmd)
{
/* that will execute under command trigger memory context */
if (cmd != NULL && cmd->before != NIL)
exec_command_triggers_internal(cmd, cmd->before, "BEFORE");

/* switch back to the command Memory Context now */
MemoryContextSwitchTo(cmd->oldmctx);
}

1. Either cmd != NULL does not need to be checked or you need to check it
before the MemoryContextSwitchTo
2. the switch to cmd->oldmctx made me very wary at first because I wasn't sure
its guaranteed to be non NULL

- why is there a special CommandTriggerContext if its not reset separately?
Should it be reset? I have to say that I dislike the api around this.

- an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema. Probably the
same problem exists elsewhere. Or is that as-designed? Would be inconsistent
with the way object names are handled.

- what does that mean?
+ cmd.objectname = NULL; /* composite object name */

- DropPropertyStmt seems to be an unused leftover?

Andres

Excerpts from Andres Freund's message of mar mar 13 08:22:26 -0300 2012:

> - I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)
> on the command trigger tuple. But then again just about nothing else does :(

If you want to do something like that, I think it's probably more
appropriate to use LockDatabaseObject than heap_lock_tuple.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi,

Andres Freund <andres(at)anarazel(dot)de> writes:
> I did a short review of what I found after merging master

Thanks!

> - I still find it strange not to fire on cascading actions

We don't build statement for cascading so we don't fire command
triggers. The user view is that there was no drop command on the sub
objects, only on the main one.

I know it's not ideal, but that's a limit we have to bite for 9.2
unfortunately.

> - I dislike the missing locking leading to strange errors uppon concurrent
> changes. But then thats just about all the rest of commands/* is handling
> it...
>
> - I think list_command_triggers should do a heap_lock_tuple(LockTupleShared)
> on the command trigger tuple. But then again just about nothing else does :(

As you say, about nothing else does. I think that's a work for another
patch.

> - ExecBeforeOrInsteadOfCommandTriggers is referenced in
> exec_command_triggers_internal comments
> - InitCommandContext comments are outdated
> - generally comments look a bit outdated

Fixed.

> - shouldn't the command trigger stuff for ALTER TABLE be done in inside
> AlterTable instead of utility.c?

Right, done.

> - you have repetitions of the following pattern:
> void
> ExecBeforeCommandTriggers(CommandContext cmd)
> {
> /* that will execute under command trigger memory context */
> if (cmd != NULL && cmd->before != NIL)
> exec_command_triggers_internal(cmd, cmd->before, "BEFORE");
>
> /* switch back to the command Memory Context now */
> MemoryContextSwitchTo(cmd->oldmctx);
> }
>
> 1. Either cmd != NULL does not need to be checked or you need to check it
> before the MemoryContextSwitchTo

I've fixed that code.

> 2. the switch to cmd->oldmctx made me very wary at first because I wasn't sure
> its guaranteed to be non NULL
>
> - why is there a special CommandTriggerContext if its not reset separately?
> Should it be reset? I have to say that I dislike the api around this.

Some call sites need to be able to call those functions a dynamic number
of times. I could add a reset boolean parameter that would mostly be
true in all call site and false in two of them (RemoveObjects,
RemoveRelations), and add a new function to just reset the memory
context then.

Or maybe you have a better idea about the ideal API here?

> - an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema. Probably the
> same problem exists elsewhere. Or is that as-designed? Would be inconsistent
> with the way object names are handled.

I'm surprised, here's an excerpt from the added regression tests:

alter function notfun(int) set schema cmd;
NOTICE: snitch: BEFORE any ALTER FUNCTION
NOTICE: snitch: BEFORE ALTER FUNCTION public notfun
NOTICE: snitch: AFTER ALTER FUNCTION cmd notfun
NOTICE: snitch: AFTER any ALTER FUNCTION

> - what does that mean?
> + cmd.objectname = NULL; /* composite object name */

User mapping and casts object names are composite, and I don't know how
to represent that in a single text structure.

> - DropPropertyStmt seems to be an unused leftover?

Seems so, cleaned out.

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

On Tuesday, March 13, 2012 09:07:32 PM Dimitri Fontaine wrote:
> Hi,
>
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I did a short review of what I found after merging master
>
> Thanks!
>
> > - I still find it strange not to fire on cascading actions
>
> We don't build statement for cascading so we don't fire command
> triggers. The user view is that there was no drop command on the sub
> objects, only on the main one.
>
> I know it's not ideal, but that's a limit we have to bite for 9.2
> unfortunately.
Hm. Especially in partially replicated scenarios I think that will bite. But
then: There will be a 9.3 at some point ;)

> > - I dislike the missing locking leading to strange errors uppon
> > concurrent changes. But then thats just about all the rest of commands/*
> > is handling it...
> >
> > - I think list_command_triggers should do a
> > heap_lock_tuple(LockTupleShared)
> >
> > on the command trigger tuple. But then again just about nothing else
> > does :(
>
> As you say, about nothing else does. I think that's a work for another
> patch.
Not sure, that way the required work is getting bigger and bigger. But I can
live with that... I think the command trigger work will make better
concurrency safeness of DDL necessary.

> > 2. the switch to cmd->oldmctx made me very wary at first because I wasn't
> > sure its guaranteed to be non NULL
> >
> > - why is there a special CommandTriggerContext if its not reset
> > separately? Should it be reset? I have to say that I dislike the api
> > around this.
>
> Some call sites need to be able to call those functions a dynamic number
> of times. I could add a reset boolean parameter that would mostly be
> true in all call site and false in two of them (RemoveObjects,
> RemoveRelations), and add a new function to just reset the memory
> context then.
> Or maybe you have a better idea about the ideal API here?
I wonder if the answer is making the API more symmetric. Seems more future-
proof in combination to being cleaner.

//create a new memory context
InitCommandContext(cmd);

if(CommandFiresTriggers(cmd)){
//still in current memory context, after all not much memory should be
allocated here
cmd.foo = bar;
//switches memory context during function execution, resets it afterwards
ExecBeforeCommandTriggers(cmd);
}

if(CommandFiresTriggers(cmd)){
cmd.zap = blub;
ExecAfterCommandTriggers(cmd);
}

//drop the memory context
CleanupCommandContext(cmd);

I find the changing of memory context in CommandFires[After]Trigger + switch
back in Exec*CommandTrigger rather bad style and I don't really see the point
anyway.

> > - an AFTER .. ALTER AGGREATE ... SET SCHEMA has the wrong schema.
> > Probably the same problem exists elsewhere. Or is that as-designed?
> > Would be inconsistent with the way object names are handled.
>
> I'm surprised, here's an excerpt from the added regression tests:
>
> alter function notfun(int) set schema cmd;
> NOTICE: snitch: BEFORE any ALTER FUNCTION
> NOTICE: snitch: BEFORE ALTER FUNCTION public notfun
> NOTICE: snitch: AFTER ALTER FUNCTION cmd notfun
> NOTICE: snitch: AFTER any ALTER FUNCTION
I was not looking at ALTER FUNCTION but ALTER AGGREGATE. And I looked wrongly.
Sorry for that.

Generally, uppon rereading, I have to say that I am not very happy with the
decision that ANY triggers are fired from other places than the specific
triggers. That seams to be a rather dangerous/confusing route to me.
Especially because sometimes errors (permissions, duplicated names, etc) are
raised differently in ANY than in specific triggers now:

postgres=# ALTER AGGREGATE bar.array_agg_union3(anyarray) SET SCHEMA public;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>,
objectname <NULL>
ERROR: aggregate bar.array_agg_union3(anyarray) does not exist

postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA
public;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>,
objectname <NULL>
ERROR: function array_agg_union3(anyarray) is already in schema "public"

postgres=# ALTER AGGREGATE public.array_agg_union3(anyarray) SET SCHEMA bar;
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>,
objectname <NULL>
NOTICE: when BEFORE, tag ALTER AGGREGATE, objectid 16415, schemaname public,
objectname array_agg_union3
NOTICE: when AFTER, tag ALTER AGGREGATE, objectid 16415, schemaname bar,
objectname array_agg_union3
NOTICE: when AFTER, tag ALTER AGGREGATE, objectid <NULL>, schemaname <NULL>,
objectname <NULL>

Andres

On Tue, Mar 13, 2012 at 5:06 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Generally, uppon rereading, I have to say that I am not very happy with the
> decision that ANY triggers are fired from other places than the specific
> triggers. That seams to be a rather dangerous/confusing route to me.

I agree. I think that's a complete non-starter.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Mar 13, 2012 at 5:06 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Generally, uppon rereading, I have to say that I am not very happy with the
>> decision that ANY triggers are fired from other places than the specific
>> triggers. That seams to be a rather dangerous/confusing route to me.
>
> I agree. I think that's a complete non-starter.

Ok, well, let me react in 2 ways here:

A. it's very easy to change and will simplify the code
B. it's been done this way for good reasons (at the time)

Specifically, I've been asked to implement the feature of blocking all
and any DDL activity on a machine in a single command, and we don't have
support for triggers on all commands (remember shared objects).

Now, as I've completed support for all interesting commands the
discrepancy between what's supported in the ANY case and in the specific
command case has reduced. If you're saying to nothing, that's good news.

Also, when calling the user's procedure from the same place in case of an
ANY command trigger or a specific one it's then possible to just hand
them over the exact same set of info (object id, name, schema name).

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

On Wed, Mar 14, 2012 at 4:27 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Also, when calling the user's procedure from the same place in case of an
> ANY command trigger or a specific one it's then possible to just hand
> them over the exact same set of info (object id, name, schema name).

Yes, I think that's an essential property of the system, here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Also, when calling the user's procedure from the same place in case of an
>> ANY command trigger or a specific one it's then possible to just hand
>> them over the exact same set of info (object id, name, schema name).
>
> Yes, I think that's an essential property of the system, here.

Ok, I've implemented that. No patch attached because I need to merge
with master again and I'm out to sleep now, it sometimes ring when being
on-call…

Curious people might have a look at my github repository where the
command_triggers branch is updated:

https://github.com/dimitri/postgres/compare/daf69e1e...e3714cb9e6

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

On 14 March 2012 21:33, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Ok, I've implemented that. No patch attached because I need to merge
> with master again and I'm out to sleep now, it sometimes ring when being
> on-call…
>
> Curious people might have a look at my github repository where the
> command_triggers branch is updated:

Will you also be committing the trigger function variable changes
shortly? I don't wish to test anything prior to this as that will
involve a complete re-test from my side anyway.

--
Thom

Thom Brown <thom(at)linux(dot)com> writes:
> Will you also be committing the trigger function variable changes
> shortly? I don't wish to test anything prior to this as that will
> involve a complete re-test from my side anyway.

It's on its way, I had to spend time elsewhere, sorry about that. With
some luck I can post a intermediate patch later this evening limited to
PLpgSQL support for your testing.

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

Andres Freund <andres(at)anarazel(dot)de> writes:
> [ ctas-01.patch ]

I'm starting to look at this now. For a patch that's supposed to
de-complicate things, it seems pretty messy :-(

One thing I soon found is that it lacks support for EXPLAIN SELECT INTO.
That used to work, but now you get

regression=# explain select * into foo from tenk1;
ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT
LINE 1: explain select * into foo from tenk1;
^

and while fixing the parse analysis for that is probably not too hard,
it would still fail to work nicely, since explain.c lacks support for
CreateTableAsStmt utility statements. I think we'd have to invent
something parallel to ExplainExecuteQuery to support this, and I really
doubt that it's worth the trouble. Does anyone have a problem with
desupporting the case?

Also, it seems to me that the patch is spending way too much effort on
trying to exactly duplicate the old error messages for various flavors
of "INTO not allowed here", and still not succeeding terribly well.
I'm inclined to just have a one-size-fits-all message in
transformSelectStmt, which will fire if intoClause hasn't been cleared
before we get there; any objections?

A couple of other cosmetic thoughts: I'm tempted to split the execution
support out into a new file, rather than bloat tablecmds.c any further;
and I'm wondering if the interface to EXECUTE INTO can't be simplified.
(It may have been a mistake to remove the IntoClause in ExecuteStmt.)

regards, tom lane

On Friday, March 16, 2012 09:54:47 PM Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > [ ctas-01.patch ]
>
> I'm starting to look at this now.
Great!

> For a patch that's supposed to de-complicate things, it seems pretty messy
:-(
Yea. It started out simple but never stopped getting more complicated. Getting
rid of the duplication still seems to make sense to me though.

> One thing I soon found is that it lacks support for EXPLAIN SELECT INTO.
> That used to work, but now you get
>
> regression=# explain select * into foo from tenk1;
> ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT
> LINE 1: explain select * into foo from tenk1;
> ^
>
> and while fixing the parse analysis for that is probably not too hard,
> it would still fail to work nicely, since explain.c lacks support for
> CreateTableAsStmt utility statements.I think we'd have to invent
> something parallel to ExplainExecuteQuery to support this, and I really
> doubt that it's worth the trouble. Does anyone have a problem with
> desupporting the case?
I am all for removing it.

> Also, it seems to me that the patch is spending way too much effort on
> trying to exactly duplicate the old error messages for various flavors
> of "INTO not allowed here", and still not succeeding terribly well.
> I'm inclined to just have a one-size-fits-all message in
> transformSelectStmt, which will fire if intoClause hasn't been cleared
> before we get there; any objections?
I was/am decidedly unhappy about the whole error message stuff. Thats what
turned the patch from removing lines to making it bigger than before.
I tried to be compatible to make sure I understood what was happening. I am
fine with making it one simple error message.

> A couple of other cosmetic thoughts: I'm tempted to split the execution
> support out into a new file, rather than bloat tablecmds.c any further;
> and I'm wondering if the interface to EXECUTE INTO can't be simplified.
> (It may have been a mistake to remove the IntoClause in ExecuteStmt.)
Hm. I vote against keeping the IntoClause stuff in ExecuteStmt. Splitting
stuff from tablecmd.c seems sensible though.
One more thing I disliked quite a bit was the duplication of the EXECUTE
handling. Do you see a way to deduplicate that?

If you give me some hints about what to change I am happy to revise the patch
myself should you prefer that.

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> One more thing I disliked quite a bit was the duplication of the EXECUTE
> handling. Do you see a way to deduplicate that?

Yeah, that's what's bugging me, too. I think a chunk of the problem is
that you're insisting on having control come back to CreateTableAs()
to perform the table creation. I'm thinking that if the table creation
were to be moved into the tuple receiver's startup routine, we could
avoid needing to get control back between ExecutorStartup and
ExecutorRun, and then all that would be required would be to call
ExecuteQuery with a different DestReceiver.

regards, tom lane

On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > One more thing I disliked quite a bit was the duplication of the EXECUTE
> > handling. Do you see a way to deduplicate that?
> Yeah, that's what's bugging me, too. I think a chunk of the problem is
> that you're insisting on having control come back to CreateTableAs()
> to perform the table creation. I'm thinking that if the table creation
> were to be moved into the tuple receiver's startup routine, we could
> avoid needing to get control back between ExecutorStartup and
> ExecutorRun, and then all that would be required would be to call
> ExecuteQuery with a different DestReceiver.
Hm. I seriously dislike doing that in the receiver. I can't really point out
why though. Unsurprisingly I like getting the control back to CreateTableAs...

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote:
>> I'm thinking that if the table creation
>> were to be moved into the tuple receiver's startup routine, we could
>> avoid needing to get control back between ExecutorStartup and
>> ExecutorRun, and then all that would be required would be to call
>> ExecuteQuery with a different DestReceiver.

> Hm. I seriously dislike doing that in the receiver. I can't really point out
> why though. Unsurprisingly I like getting the control back to CreateTableAs...

I don't see the argument. Receiver startup functions are intended to do
exactly this type of thing. I'd be okay with leaving it in
CreateTableAs if it didn't contort the code to do so, but what we have
here is precisely that we've got to contort the interface with prepare.c
to do it that way.

(It also occurs to me that moving this work into the DestReceiver might
make things self-contained enough that we could continue to support
EXPLAIN SELECT INTO with not an undue amount of pain.)

Something else I just came across is that there are assorted places that
are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c,
and those have got to treat CreateTableAsStmt similarly. We could just
add more code in each of those places. I'm wondering though if it would
be a good idea to invent an abstraction layer, to wit a utility.c
function along the lines of "Query *UtilityContainsQuery(Node
*parsetree)", which would centralize the knowledge of exactly which
utility statements are like this and how to dig the Query out of them.
It's only marginally attractive unless there's a foreseeable reason
why we'd someday have more than two of them; but on the other hand,
just formalizing the concept that some utility statements are like
this might be a good thing. (Actually, as I type this I wonder whether
COPY (SELECT ...) isn't a member of this class too, and whether we don't
have bugs from the fact that it's not being treated like EXPLAIN.)
Thoughts?

regards, tom lane

On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote:
> >> I'm thinking that if the table creation
> >> were to be moved into the tuple receiver's startup routine, we could
> >> avoid needing to get control back between ExecutorStartup and
> >> ExecutorRun, and then all that would be required would be to call
> >> ExecuteQuery with a different DestReceiver.
> >
> > Hm. I seriously dislike doing that in the receiver. I can't really point
> > out why though. Unsurprisingly I like getting the control back to
> > CreateTableAs...
>
> I don't see the argument. Receiver startup functions are intended to do
> exactly this type of thing. I'd be okay with leaving it in
> CreateTableAs if it didn't contort the code to do so, but what we have
> here is precisely that we've got to contort the interface with prepare.c
> to do it that way.
Hm.

> (It also occurs to me that moving this work into the DestReceiver might
> make things self-contained enough that we could continue to support
> EXPLAIN SELECT INTO with not an undue amount of pain.)

> Something else I just came across is that there are assorted places that
> are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c,
> and those have got to treat CreateTableAsStmt similarly.
Hm. Is that so? As implemented in my version the planner just sees a plain
statement instead of a utility statement. Am I missing something?

I am not even sure why the planner ever needs to see ExplainStmts? Protocol
level prepares? Shouldn't those top level nodes be simply removed once?

> We could just
> add more code in each of those places. I'm wondering though if it would
> be a good idea to invent an abstraction layer, to wit a utility.c
> function along the lines of "Query *UtilityContainsQuery(Node
> *parsetree)", which would centralize the knowledge of exactly which
> utility statements are like this and how to dig the Query out of them.
> It's only marginally attractive unless there's a foreseeable reason
> why we'd someday have more than two of them; but on the other hand,
> just formalizing the concept that some utility statements are like
> this might be a good thing.
If its really necessary to do that I think that would be a good idea. Alone
the increased greppablility/readablility seems to be worth it.

> (Actually, as I type this I wonder whether
> COPY (SELECT ...) isn't a member of this class too, and whether we don't
> have bugs from the fact that it's not being treated like EXPLAIN.)
> Thoughts?
Hm. On a first glance the planner also never sees the content of a CopyStmt...

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote:
>> Something else I just came across is that there are assorted places that
>> are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c,
>> and those have got to treat CreateTableAsStmt similarly.

> Hm. Is that so? As implemented in my version the planner just sees a plain
> statement instead of a utility statement. Am I missing something?

Well, the work flow for EXPLAIN is:

parse analysis: recursively do parse analysis on contained query
plan: do nothing
execution: call planner on contained query, then optionally run it

and the reason for doing it that way is explained by
transformExplainStmt:

* EXPLAIN is like other utility statements in that we emit it as a
* CMD_UTILITY Query node; however, we must first transform the contained
* query. We used to postpone that until execution, but it's really necessary
* to do it during the normal parse analysis phase to ensure that side effects
* of parser hooks happen at the expected time.

ISTM that argument applies just as much to CREATE TABLE AS, especially
in view of the fact that we're restructuring the SELECT INTO case, in
which parse analysis of the SELECT certainly does happen early. It's
also not clear to me why it wouldn't apply to COPY (SELECT ...).

I'm not going to touch the COPY (SELECT ...) issue right now, but
somebody ought to go back and check up on the exact user-visible bugs
that motivated moving EXPLAIN's parse analysis processing. (I suspect
it had to do with plpgsql variable processing, but too lazy to go look
right now.) If there's a plausible use case where similar bugs could
be exhibited in COPY, we're going to have to restructure that too.

regards, tom lane

While looking at this I also noticed that DECLARE CURSOR uses a
structure that's randomly different in yet a third way: we start
with a utility statement containing a query, and then flip that
upside down so that the SELECT Query contains a utility statement!

I have a vague feeling that I'm the one who's guilty of that hack, too.

I'm not sure that anybody cares about being able to fire command
triggers on DECLARE CURSOR, but just from a consistency standpoint it
would make sense for this to work more like EXPLAIN and CREATE TABLE AS.
So that convinces me that UtilityContainsQuery() would be a good thing,
and I'll add that to the patch. (Actually changing DECLARE CURSOR seems
like a separate patch, though.)

regards, tom lane

On Saturday, March 17, 2012 06:45:27 PM Tom Lane wrote:
> I'm not sure that anybody cares about being able to fire command
> triggers on DECLARE CURSOR
I actually think it would make sense to explicitly not fire command triggers
there given that DECLARE CURSOR actually potentially is somewhat performance
relevant.

> , but just from a consistency standpoint it
> would make sense for this to work more like EXPLAIN and CREATE TABLE AS.
> So that convinces me that UtilityContainsQuery() would be a good thing,
> and I'll add that to the patch. (Actually changing DECLARE CURSOR seems
> like a separate patch, though.)
Aggreed on that.

Andres

I've found a couple more issues in the CTAS patch:

1. Previous versions delivered a "SELECT n" command tag for either
spelling of the command:

regression=# select * into t1 from int8_tbl;
SELECT 6
regression=# create table t2 as select * from int8_tbl;
SELECT 6

With the patch I get

regression=# select * into t1 from int8_tbl;
SELECT 0 0
regression=# create table t2 as select * from int8_tbl;
CREATE TABLE AS

The first of these is particularly unfortunate since it's outright lying
as to the number of rows processed.

I'm not sure what we should do instead. We have gotten push-back before
anytime we changed the command tag for an existing command (and in fact
it seems that we intentionally added the rowcount display in 9.0, which
means there are people out there who care about that functionality).
On the other hand, the traditional output for CREATE TABLE AS doesn't
seem to satisfy the principle of least astonishment. A third
consideration is that if we are pushing CREATE TABLE AS as the preferred
spelling, people will probably complain if it omits functionality that
SELECT INTO provides; so I'm not sure that "SELECT n" in one case and
"CREATE TABLE AS" in the other would be a good idea either. Any
opinions what to do here?

2. Historically, CREATE RULE has allowed a rule action to be SELECT INTO
(though not CREATE TABLE AS). Currently the patch is throwing an error
for that. This seems like something that might not be worth fixing,
though. It's fairly hard to conceive of a use-case for such a rule,
since it would work only once before starting to throw "table already
exists" errors. How much do we care about preserving backward
compatibility here?

regards, tom lane