Re: review: CHECK FUNCTION statement - Mailing list pgsql-hackers
From | Pavel Stehule |
---|---|
Subject | Re: review: CHECK FUNCTION statement |
Date | |
Msg-id | CAFj8pRAVEgC2gaFG+ji2tgFFiVSzTUS93=7pmQ8JgmCWJLBqAA@mail.gmail.com Whole thread Raw |
In response to | Re: review: CHECK FUNCTION statement ("Albe Laurenz" <laurenz.albe@wien.gv.at>) |
Responses |
Re: review: CHECK FUNCTION statement
|
List | pgsql-hackers |
Hello 2011/11/29 Albe Laurenz <laurenz.albe@wien.gv.at>: > Pavel Stehule wrote: >> I am sending updated patch, that implements a CHECK FUNCTION and CHECK >> TRIGGER statements. >> >> This patch is significantly redesigned to previous version (PL/pgSQL >> part) - it is more readable, more accurate. There are new regress >> tests. >> >> Please, can some English native speaker fix doc and comments? > >> ToDo: >> >> CHECK FUNCTION search function according to function signature - it >> should be changes for using a actual types - it can be solution for >> polymorphic types and useful tool for work with overloaded functions - >> when is not clean, that function was executed. >> >> check function foo(int, int); >> NOTICE: checking function foo(variadic anyarray) >> ... >> >> and maybe some support for named parameters >> check function foo(name text, surname text); >> NOTICE: checking function foo(text, text, text, text) >> ... > > I think that CHECK FUNCTION should work exactly like DROP FUNCTION > in these respects. > > Submission review: > ------------------ > > The patch is context diff, applies with some offsets, contains > regression tests and documentation. > > The documentation should be expanded, the doc for CHECK FUNCTION > is only a stub. It should describe the procedure and what is checked. > That would also make reviewing easier. > I think that some documentation should be added to plhandler.sgml. > There is a spelling error (statemnt) in the docs. > > Usability review: > ----------------- > > If I understand right, the goal of CHECK FUNCTION is to find errors in > the function definition without actually having to execute it. > The patch tries to provide this for PL/pgSQL. > > There hasn't been any discussion on the list, the patch was just posted, > so I can't say that we want that. Tom added it to the commitfest page, > so there's one important voice against dismissing it right away :^) This feature was transformed from plpgsql_lint contrib module. So there was a voises so this functionality should be in contrib module as minimum http://archives.postgresql.org/pgsql-hackers/2011-07/msg00900.php http://archives.postgresql.org/pgsql-hackers/2011-07/msg01035.php Contrib module has one disadvantage - it cannot be used in combination with other plpgsql extensions like edb debugger or edb profiler. So I rewrote it as core plpgsql patch. It was a plpgsql.prepare_plans feature. This idea was rejected and replaced by CHECK FUNCTION statement Tom propossed a syntax http://archives.postgresql.org/pgsql-hackers/2011-09/msg00549.php http://archives.postgresql.org/pgsql-hackers/2011-09/msg00563.php > > I don't understand the functional difference between a "validator function" > and a "check function" as proposed by this patch. I am probably missing > something, but why couldn't these checks be added to function validation > when check_function_bodies is set? > A new "CHECK FUNCTION" statement could simply call the validator function. A validation function is not simple now - and check feature increase a complexity. Other problem with validator function is their polymorphic interface. > > I don't see any pg_dump support in this patch, and PL/pgSQL probably doesn't > need that, but I think pg_dump support for CREATE LANGUAGE would have to > be added for other PLs. I have to recheck it > > I can't test if the functionality is complete because I can't get it to > run (see below). sorry - I'll look on it > > Feature test: > ------------- > > I can't really test the patch because initdb fails: > > $ initdb -E UTF8 --locale=de_DE.UTF-8 --lc-messages=en_US.UTF-8 -U postgres /postgres/cvs/dbhome > The files belonging to this database system will be owned by user "laurenz". > This user must also own the server process. > > The database cluster will be initialized with locales > COLLATE: de_DE.UTF-8 > CTYPE: de_DE.UTF-8 > MESSAGES: en_US.UTF-8 > MONETARY: de_DE.UTF-8 > NUMERIC: de_DE.UTF-8 > TIME: de_DE.UTF-8 > The default text search configuration will be set to "german". > > creating directory /postgres/cvs/dbhome ... ok > creating subdirectories ... ok > selecting default max_connections ... 100 > selecting default shared_buffers ... 32MB > creating configuration files ... ok > creating template1 database in /postgres/cvs/dbhome/base/1 ... ok > initializing pg_authid ... ok > initializing dependencies ... ok > creating system views ... ok > loading system objects' descriptions ... ok > creating collations ... ok > creating conversions ... ok > creating dictionaries ... ok > setting privileges on built-in objects ... ok > creating information schema ... ok > loading PL/pgSQL server-side language ... FATAL: could not load library "/postgres/cvs/pg92/lib/plpgsql.so": /postgres/cvs/pg92/lib/plpgsql.so:undefined symbol: plpgsql_delete_function > STATEMENT: CREATE EXTENSION plpgsql; > > child process exited with exit code 1 > initdb: removing data directory "/postgres/cvs/dbhome" > > Coding review: > -------------- > > The patch compiles without warnings. > The comments in the code should be revised, they are bad English. > I can't say if there should be more of them -- I don't know this part of > the code well enough to have a well-founded opinion. > > I don't think there are any portability issues, but I could not test it. > > There are a lot of small changes to pl/plpgsql/src/pl_exec.c, are they all > necessary? For example, why was copy_plpgsql_datum renamed to > plpgsql_copy_datum? yes, it's necessary - a implementation is in new file and there is necessary call a functions from pg_compile and pg_exec files - checking is between compilation and execution - so some functions should not be static now. All plpgsql public functions should start with plpgsql_ prefix. It is reason for renaming. > > I'll mark the patch as "Waiting on Author". I'll look on it this night Regards Pavel > > Yours, > Laurenz Albe >
pgsql-hackers by date: