Re: Command Triggers, patch v11

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