Re: Command Triggers, patch v11 - Mailing list pgsql-hackers

Hi,

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

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

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

Thom Brown <thom@linux.com> writes:
> I just tried building the docs with your patch and it appears
> doc/src/sgml/ref/allfiles.sgml hasn't been updated with the necessary
> references for alterCommandTrigger, createCommandTrigger and
> dropCommandTrigger.
>
> Also in ref/alter_command_trigger.sgml, you define SQL-CREATETRIGGER.
> Shouldn't this be SQL-CREATECOMMANDTRIGGER?  And there also appears to
> be orphaned text in the file too, such as "Forbids the execution of
> any DDL command".  And there's a stray </para> on line 299.
>
> I attach updated versions of both of those files, which seems to fix
> all these problems.

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

Thom Brown <thom@linux.com> writes:
> I've just noticed there's an issue with
> doc/src/sgml/ref/alter_command_trigger.sgml too.  It uses <indexterm
> zone="sql-altertrigger"> which should be sql-altercommandtrigger. (as
> attached)

Included.

Thom Brown <thom@linux.com> writes:
> And upon trying to test the actual feature, it didn't work for me at
> all.  I thought I had applied the patch incorrectly, but I hadn't, it
> was the documentation showing the wrong information.  The CREATE
> COMMAND TRIGGER page actually just says CREATE TRIGGER.... BEFORE
> COMMAND <command>, which isn't the correct syntax.

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

> Also the examples on the page are incorrect in the same regard.  When
> I tested it with the correction, I got an error saying that the
> function used had to return void, but the example uses bool.  Upon
> also changing this, the example works as expected.

Fixed too.

Thom Brown <thom@linux.com> writes:
> Is there any reason why the list of commands that command triggers can
> be used with isn't in alphabetical order?  Also it appears to show

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

> CREATE/ALTER/DROP TYPE_P, and the same for CONVERSION_P and DOMAIN_P.
> I'm assuming these are typos?  They also appear on DROP COMMAND
> TRIGGER.

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

> The ALTER COMMAND TRIGGER page also doesn't show which commands it can
> be used against.  Perhaps, rather than repeat the list, there could be
> a note to say that a list of valid commands can be found on the CREATE
> COMMAND TRIGGER page?

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

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