Re: review: CHECK FUNCTION statement

2011/11/29 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> updated patch:
>
> * recheck compilation and initdb
> * working routines moved to pl_exec.c
> * add entry to catalog.sgml about lanchecker field
> * add node's utils

+ pg_dump support

Pavel

>
> Regards
>
> Pavel Stehule
>
> 2011/11/29 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)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 :^)
>>
>> 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.
>>
>> 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 can't test if the functionality is complete because I can't get it to
>> run (see below).
>>
>> 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?
>>
>> I'll mark the patch as "Waiting on Author".
>>
>> Yours,
>> Laurenz Albe
>>
>