Re: review: CHECK FUNCTION statement

2011/12/7 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> 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.

this system is based on error messages that generates a plpgsql engine
or bison engine. I can correct only a few percent from these messages
:(

internally I didn't wrote a compiler or plpgsql checker - this is just
tool that can emit some plpgsql interpret subprocess - and when these
subprocesses raises exceptions, then takes their messages.

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

this is question - this will check all functions in postgres.It's 2421
function, so one criterium as minimum should be good idea.

We can remove buildin functions from list - so it will check all
function in database.

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

IN should be syntactic sugar

> 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 TRIGGER listed
>  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".

please, can you send a correction to documentation or error messages?

I am not able to write documentation

Regards

Pavel
>
> Yours,
> Laurenz Albe