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
>> Please, can some English native speaker fix doc and comments?
>> 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
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
Tom propossed a syntax
> 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
> 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
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
> Laurenz Albe
In response to
pgsql-hackers by date
|Next:||From: Andrew Dunstan||Date: 2011-11-29 15:01:49|
|Subject: Re: Review of VS 2010 support patches|
|Previous:||From: Robert Haas||Date: 2011-11-29 14:17:52|
|Subject: Re: synchronous commit vs. hint bits|