2011/12/7 Albe Laurenz <laurenz.a...@wien.gv.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers