Re: review: CHECK FUNCTION statement

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
>