Pavel Stehule wrote:
> there is a updated patch.
>
> it support multi check, options and custom check functions are not
> supported yet. I don't plan to implement custom check functions in
> this round - I has not any example of usage - but we have agreement on
> syntax and behave, so this should not be problem. I changed reporting
> - from exception to warnings.
The patch applies and builds cleanly.
The syntax error messages are still inadequate; all I can get is
'syntax error at or near "%s"'. They should be more detailed.
Many other messages and code comments are in bad English.
It might be a good idea to add some regression tests for the
CHECK FUNCTION ALL variants.
Functionality:
--------------
I noticed an oddity:
postgres=# CHECK FUNCTION ALL;
ERROR: syntax error at or near ";"
LINE 1: CHECK FUNCTION ALL; ^
postgres=# CHECK FUNCTION ALL IN LANGUAGE plpgsql;
NOTICE: nothing to check
postgres=# CHECK FUNCTION ALL IN SCHEMA pg_catalog;
[prints lots of NOTICEs]
According to the syntax diagram and my intuition CHECK FUNCTION ALL
without additional clauses should work.
Regarding the syntax: I know I suggested it myself, but after several
times of typing "IN LANGUAGE plpgsql" I think that omitting the "IN"
would be better and more like other commands (e.g. CREATE FUNCTION).
It is a pity that the CHECK FUNCTION ALL variants will not check
trigger functions, but I understand the difficulty -- it would
require checking all trigger functions on all tables where they
occur in a trigger.
I think that the checker function should be shown in psql's
\dL+ output.
Barring these little gripes, the functionality seems "ready for
committer" from my point of view.
Code review:
------------
I do not feel competent for a thorough code review.
Documentation:
--------------
This is where I see the greatest shortcomings.
- The documentation for the system catalog pg_pltemplate should be extended to include tmplchecker.
- The documentation patch for CREATE LANGUAGE is plain wrong and contains a syntax error.
- CHECK FUNCTION and CHECK TRIGGER should be treated as different SQL statements. It is misleading to have CHECK
TRIGGERlisted under CHECK FUNCTION. If they have to be together, the statement should be called "CHECK" and not "CHECK
TRIGGER",but I think they should be separate.
- There is still no documentation patch for plhandler.sgml.
I think that at least the documentation should be improved before
I am ready to set this as "ready for committer".
Yours,
Laurenz Albe
