Re: review: CHECK FUNCTION statement

Hello all,

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?

Usage is very simply

postgres=# \d t1
Table "public.t1"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
b | integer |
Triggers:
t1_f1 BEFORE INSERT ON t1 FOR EACH ROW EXECUTE PROCEDURE f1_trg()

postgres=# \sf+ f1_trg
CREATE OR REPLACE FUNCTION public.f1_trg()
RETURNS trigger
LANGUAGE plpgsql
1 AS $function$
2 begin
3 new.a := new.a + 10;
4 new.b := new.b + 10;
5 new.c := 30;
6 return new;
7 end;
8 $function$
postgres=# check trigger t1_f1 on t1;
NOTICE: checking function "f1_trg()"
ERROR: record "new" has no field "c"
CONTEXT: checking of PL/pgSQL function "f1_trg" line 5 at assignment

Checker handler should be called explicitly

postgres=# select plpgsql_checker('f1'::regproc, 0);
ERROR: column "c" of relation "t1" does not exist
LINE 1: update t1 set c = 30
^
QUERY: update t1 set c = 30
CONTEXT: checking of PL/pgSQL function "f1" line 4 at SQL statement

or (check or plpgsql custom functions)

DO $$
DECLARE r regprocedure;
BEGIN
FOR r IN SELECT p.oid
FROM pg_catalog.pg_proc p
LEFT JOIN pg_catalog.pg_namespace n ON
n.oid = p.pronamespace
LEFT JOIN pg_catalog.pg_language l ON
l.oid = p.prolang
WHERE l.lanname = 'plpgsql'
AND n.nspname <> 'pg_catalog'
AND n.nspname <> 'information_schema'
AND p.prorettype <>
'pg_catalog.trigger'::pg_catalog.regtype
LOOP
RAISE NOTICE 'check %', r;
PERFORM plpgsql_checker(r, 0);
END LOOP;
END;
$$;

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)
...

Regards

Pavel Stehule

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

Hello

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 :^)

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
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/11/29 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>> 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 don't think renaming is necessary. plpgsql is a standalone shared
library and so its symbols don't matter to anybody but itself.

Possibly a larger question, though, is whether you really need a new
source file. If that results in having to export functions that
otherwise could stay static, maybe it's not the best choice.

regards, tom lane

2011/11/29 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2011/11/29 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>>> 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 don't think renaming is necessary.  plpgsql is a standalone shared
> library and so its symbols don't matter to anybody but itself.
>
> Possibly a larger question, though, is whether you really need a new
> source file.  If that results in having to export functions that
> otherwise could stay static, maybe it's not the best choice.

This patch was originally in pl_exec.c but this file has a 6170 lines
and checking adds 1092 lines - so I moved it to new file

It has little bit different semantics, but it is true, so checking
hardly depends on routines from pl_exec - routines for variable's
management.

I have no problem to move it back. I reduces original patch little bit.

Some refactoring of pl_exec should be nice - a management of row,
record variables and array fields is part that can be shared with
SQL/PSM interpret. But I have not idea how it realize.

Regards

Pavel

>
>                        regards, tom lane
>

Excerpts from Pavel Stehule's message of mar nov 29 14:37:24 -0300 2011:
> 2011/11/29 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> > I don't think renaming is necessary.  plpgsql is a standalone shared
> > library and so its symbols don't matter to anybody but itself.
> >
> > Possibly a larger question, though, is whether you really need a new
> > source file.  If that results in having to export functions that
> > otherwise could stay static, maybe it's not the best choice.
>

> Some refactoring of pl_exec should be nice - a management of row,
> record variables and array fields is part that can be shared with
> SQL/PSM interpret. But I have not idea how it realize.

I proposed at the PL summit that perhaps we should have some sort of PL
lib that would be shared by plpgsql and plpsm, to reduce code
duplication.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

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
>

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
>>
>

Pavel Stehule wrote:
> 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

Documentation:
--------------

This patch has no documentation for CHECK FUNCTION or CHECK TRIGGER.
The last patch had at least something.
"\h check function" in psql does not show anything.

The patch should also add documentation about the handler function
to plhandler.sgml. In particular, the difference between the
validator function and the check function should be pointed out.

Usability:
----------

Do I understand right that the reason why the check function is
different from the validator function is that it would be more difficult
to add the checks to the validator function?

Is that a good enough argument? From a user's perspective it is
difficult to see why some checks are performed at function creation
time, while others have to be explicitly checked with CHECK FUNCTION.
I think it would be much more intuitive if CHECK FUNCTION does
the same as function validation with check_function_bodies on.

Submission, Compilation, Regression tests:
------------------------------------------

The patch applies and compiles fine and passes regression tests.
The tests cover the functionality.
"initdb" succeeds.

pg_dump:
--------

pg_dump support does not work right.
If I create a language like this:

CREATE LANGUAGE mylang HANDLER plpgsql_call_handler
INLINE plpgsql_inline_handler
VALIDATOR plpgsql_validator
CHECK plpgsql_checker;
the dump will contain:
CREATE OR REPLACE PROCEDURAL LANGUAGE mylang;

This is not a problem of this patch though (same in 9.1);
it seems that pg_dump support for languages without definition
in pg_pltemplate is broken in general.

Feature test:
-------------

CHECK FUNCTION and CHECK TRIGGER work, I couldn't crash it.

Error messages could be better:
CHECK TRIGGER atrigger;
ERROR: syntax error at or near ";"
LINE 1: CHECK TRIGGER atrigger;
^
Something like "expected keyword 'ON'" might be nice.

There are a lot of things that CHECK FUNCTION does not check, some
examples:

1)

CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer
LANGUAGE plpgsql STRICT AS
$$DECLARE j integer;
BEGIN
IF i=1 THEN
FOR I IN 1..4 BY -1 LOOP
RAISE NOTICE '%', i;
END LOOP;
RETURN -1;
ELSE
RETURN 2*i;
END IF;
END;$$;

CHECK FUNCTION t(integer); -- no error

SELECT t(1);
ERROR: BY value of FOR loop must be greater than zero
CONTEXT: PL/pgSQL function "t" line 4 at FOR with integer loop variable

2)

CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer
LANGUAGE plpgsql STRICT AS
$$DECLARE j integer;
BEGIN
IF i=1 THEN
j=9999999999999999999;
RETURN j;
ELSE
RETURN 2*i;
END IF;
END;$$;

CHECK FUNCTION t(integer); -- no error

SELECT t(1);
ERROR: value "9999999999999999999" is out of range for type integer
CONTEXT: PL/pgSQL function "t" line 4 at assignment

3)

CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer
LANGUAGE plpgsql STRICT AS
$$DECLARE j atable;
BEGIN IF i=1 THEN
j=12;
RETURN j;
ELSE
RETURN 2*i;
END IF;
END;$$;

CHECK FUNCTION t(integer); -- no error

SELECT t(1);
ERROR: cannot assign non-composite value to a row variable
CONTEXT: PL/pgSQL function "t" line 3 at assignment

4)

CREATE TABLE atable(
id integer PRIMARY KEY,
val text NOT NULL
);

INSERT INTO atable VALUES (1, 'eins');

CREATE OR REPLACE FUNCTION atrigger() RETURNS trigger
LANGUAGE plpgsql STRICT AS
$$BEGIN
NEW.id=22;
RETURN NEW;
END;$$;

CREATE TRIGGER atrigger AFTER DELETE ON atable FOR EACH ROW
EXECUTE PROCEDURE atrigger();

CHECK TRIGGER atrigger ON atable; -- no error
NOTICE: checking function "atrigger()"

DELETE FROM atable;
ERROR: record "new" is not assigned yet
DETAIL: The tuple structure of a not-yet-assigned record is indeterminate.
CONTEXT: PL/pgSQL function "atrigger" line 2 at assignment

I can try and come up with more if desired.

Maybe case 2) and 4) cannot reasonably be covered.

It is probably very hard to check everything possible, but the
usefulness of CHECK FUNCTION is proportional to the number of
cases covered.

I'll mark the patch as "Waiting on Author" until there is more
documentation, I understand the answers to the questions
raised in "usability" above, and until it is agreed that the things
checked are sufficient.

Yours,
Laurenz Albe

"Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> writes:
> Do I understand right that the reason why the check function is
> different from the validator function is that it would be more difficult
> to add the checks to the validator function?

> Is that a good enough argument? From a user's perspective it is
> difficult to see why some checks are performed at function creation
> time, while others have to be explicitly checked with CHECK FUNCTION.
> I think it would be much more intuitive if CHECK FUNCTION does
> the same as function validation with check_function_bodies on.

I think the important point here is that we need to support more than
one level of validation, and that the higher levels can't really be
applied by default in CREATE FUNCTION because they may fail on perfectly
valid code.

The real reason why we need a separate check function is that the API
for validators doesn't include any parameter about check level.

It's conceivable that instead of adding a check-function entry point,
we could generalizee check_function_bodies into a more-than-2-level
setting, and expect validators to pay attention to the GUC's value
when deciding how aggressively to validate. However, it's far from
clear to me that that's a more usable definition than having a separate
CHECK FUNCTION command. An example of where a separate CHECK command
could come in handy is: you just did some ALTER TABLE commands, and now
you would like to check if your functions all still work with the
modified table schemas.

> [ snip examples of some additional checks that could be made ]
> It is probably very hard to check everything possible, but the
> usefulness of CHECK FUNCTION is proportional to the number of
> cases covered.

I don't think that the initial patch needs to check everything that
could conceivably be checked. We can always add more checking later.
The initial patch obviously has to create the infrastructure for
optional checking, and the specific check that Pavel wants to add
is to run parse analysis on each SQL statement in a plpgsql function.
That seems to me to be a well-defined and useful check, so I think the
scope of the patch is entirely adequate for now.

A bigger issue is that once you think about more than one kind of check,
it becomes apparent that we might need some user-specifiable options to
control which checks are applied. And I see no provision for that here.
This is not something we can add later, at least not without breaking
the API for the check function --- and if we're willing to break API,
why not just add some more parameters to the validator and avoid having
a second function?

On the whole, it might not be a bad idea to have two allowed signatures
for the validator function, rather than inventing an additional column
in pg_language. But the fundamental point IMHO is that there needs to
be a provision to pass language-dependent validation options to the
function, whether it's the existing validator or a separate checker
entry point.

regards, tom lane

On Wed, Nov 30, 2011 at 10:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> On the whole, it might not be a bad idea to have two allowed signatures
> for the validator function, rather than inventing an additional column
> in pg_language.  But the fundamental point IMHO is that there needs to
> be a provision to pass language-dependent validation options to the
> function, whether it's the existing validator or a separate checker
> entry point.

Something like:

CHECK FUNCTION proname(proargs) WITH (...fdw-style elastic options...)

?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello

>
> CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer
>  LANGUAGE plpgsql STRICT AS
>  $$DECLARE j integer;
>    BEGIN
>      IF i=1 THEN
>        FOR I IN 1..4 BY -1 LOOP
>           RAISE NOTICE '%', i;
>        END LOOP;
>        RETURN -1;
>      ELSE
>        RETURN 2*i;
>      END IF;
>    END;$$;
>
> CHECK FUNCTION t(integer); -- no error
>
> SELECT t(1);
> ERROR:  BY value of FOR loop must be greater than zero
> CONTEXT:  PL/pgSQL function "t" line 4 at FOR with integer loop variable
>
> 2)
>
> CREATE OR REPLACE FUNCTION t(i integer) RETURNS integer
>  LANGUAGE plpgsql STRICT AS
>  $$DECLARE j integer;
>    BEGIN
>      IF i=1 THEN
>        j=9999999999999999999;
>        RETURN j;
>      ELSE
>        RETURN 2*i;
>      END IF;
>    END;$$;
>
> CHECK FUNCTION t(integer); -- no error
>
> SELECT t(1);
> ERROR:  value "9999999999999999999" is out of range for type integer
> CONTEXT:  PL/pgSQL function "t" line 4 at assignment
>

This kind of check are little bit difficult. It is solveable but I
would to have a skelet in core, and then this skelet can be enhanced
step by step.

Where is problem? PL/pgSQL usually don't work with numeric constant.
Almost all numbers are expressions - and checking function ensure only
semantic validity of expression, but don't try to evaluate expression.
So isn't possible to check runtime errors now.

Regards

Pavel

Excerpts from Tom Lane's message of mié nov 30 12:53:42 -0300 2011:

> A bigger issue is that once you think about more than one kind of check,
> it becomes apparent that we might need some user-specifiable options to
> control which checks are applied. And I see no provision for that here.
> This is not something we can add later, at least not without breaking
> the API for the check function --- and if we're willing to break API,
> why not just add some more parameters to the validator and avoid having
> a second function?

How about

CHECK (parse, names=off) FUNCTION foobar(a, b, c)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Nov 30, 2011 at 10:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> On the whole, it might not be a bad idea to have two allowed signatures
>> for the validator function, rather than inventing an additional column
>> in pg_language. But the fundamental point IMHO is that there needs to
>> be a provision to pass language-dependent validation options to the
>> function, whether it's the existing validator or a separate checker
>> entry point.

> Something like:
> CHECK FUNCTION proname(proargs) WITH (...fdw-style elastic options...)

Great minds think alike ... that was pretty much exactly the syntax that
was in the back of my mind.

regards, tom lane

2011/11/30 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Tom Lane's message of mié nov 30 12:53:42 -0300 2011:
>
>> A bigger issue is that once you think about more than one kind of check,
>> it becomes apparent that we might need some user-specifiable options to
>> control which checks are applied.  And I see no provision for that here.
>> This is not something we can add later, at least not without breaking
>> the API for the check function --- and if we're willing to break API,
>> why not just add some more parameters to the validator and avoid having
>> a second function?
>
> How about
>
> CHECK (parse, names=off) FUNCTION foobar(a, b, c)

this syntax is relative consistent with EXPLAIN, is it ok for all?

Pavel

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/11/30 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>> How about
>> CHECK (parse, names=off) FUNCTION foobar(a, b, c)

> this syntax is relative consistent with EXPLAIN, is it ok for all?

It seems pretty awkward to me, particularly putting the options before
the second keyword of the command --- that could bite us if we ever want
some other flavors of CHECK command. I prefer Robert's suggestion of a
WITH clause at the end.

regards, tom lane

2011/11/30 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2011/11/30 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>>> How about
>>> CHECK (parse, names=off) FUNCTION foobar(a, b, c)
>
>> this syntax is relative consistent with EXPLAIN, is it ok for all?
>
> It seems pretty awkward to me, particularly putting the options before
> the second keyword of the command --- that could bite us if we ever want
> some other flavors of CHECK command.  I prefer Robert's suggestion of a
> WITH clause at the end.

we can provide both versions - as can be fine for people. Is is simple
in parser. I like both variants and I am thinking so much more
important is a API of checker function and behave of CHECK FUNCTION
statement.

Just idea - don't kill me :). Because CHECK FUNCTION is not
destructive , then complete signature is not necessary, and when
function name is unique, then parameters should be optional - it can
be comfortable for manual work, so just CHECK FUNCTION name; can work.
I see a usage for option - a entering parameter's values instead
signature. When I started with overloaded functions, sometimes I had a
problem with identification of function that was executed - CHECK
FUNCTION can help

CHECK FUNCTION name(10,20) WITH (values);
Notice: checking function name(int, int, int default 20)

I would to design API of checker function be friendly to direct call.
There was some ideas to design CHECK FUNCTION for possibility to check
all functions in schema or language. It should be, but we have a
inline statement and system catalog, so anybody can write own scripts
per your requests. And It was one point to decision for separate
checker function from validate function.

Regards

Pavel

>
>                        regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/11/30 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> It seems pretty awkward to me, particularly putting the options before
>> the second keyword of the command --- that could bite us if we ever want
>> some other flavors of CHECK command. I prefer Robert's suggestion of a
>> WITH clause at the end.

> we can provide both versions - as can be fine for people. Is is simple
> in parser. I like both variants and I am thinking so much more
> important is a API of checker function and behave of CHECK FUNCTION
> statement.

I think you missed my point: I don't want the options list at the front
because I'm afraid it will prevent us from making good extensions in the
future. Offering both syntaxes does not fix that.

> Just idea - don't kill me :). Because CHECK FUNCTION is not
> destructive , then complete signature is not necessary, and when
> function name is unique, then parameters should be optional - it can
> be comfortable for manual work, so just CHECK FUNCTION name; can work.

Well, there was some discussion of having a "bulk check" or wildcard
capability in the CHECK command, but this seems like an awfully
constricted version of that.

The thing I'd prefer to see in the first cut is some notation for "check
all functions owned by me that are in language FOO". The reason for the
language restriction is that if we think the options are
language-specific, there's no reason to believe that different
validators would accept the same options.

regards, tom lane

Hello

I rechecked a possibility to use a validator function together with
checker function.

The main issue is a different interface of both functions. Validator
needs just function oid and uses global variable
check_function_bodies. Checker function needs function oid and
relation oid (possible some other params). When we mix these two
functions together we need a

validator(oid) or validator(oid, oid, variadic "any")

one parameter function is old validator, three parameters function is checker.

Question:

What is a correct signature for this function? We cannot use a
overloading, because we can have only one validator function per
language. We can change a validator to support both forms, and we have
to be carefully and correct if we will work with our validators(3 and
more params) or foreign validators (1 param). We should to support
both (compatibility reasons). We are not careful about validators now
- there are some places, where one parameter is hardly expected - this
should be changed. So using validator for checking doesn't mean
smaller patch, but is true, so these functions has similar semantic -
validator is usually "low level" checker.

What is your opinion?

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> I rechecked a possibility to use a validator function together with
> checker function.

> The main issue is a different interface of both functions. Validator
> needs just function oid and uses global variable
> check_function_bodies. Checker function needs function oid and
> relation oid (possible some other params). When we mix these two
> functions together we need a

> validator(oid) or validator(oid, oid, variadic "any")

Right, although if you want it to be callable from SQL I think that
variadic "any" is too loose.

> What is a correct signature for this function? We cannot use a
> overloading, because we can have only one validator function per
> language.

So? You have one validator function, it has either signature;
if it has the old signature then CHECK isn't supported by the language.
We have plenty of examples of this sort of thing already.

One issue that would need to be considered is how the validator tells
the difference between a CREATE FUNCTION call and a CHECK call with
default parameters (no WITH clause). Those shouldn't do exactly the
same thing, presumably. Maybe that's a sufficient reason to have two
entry points.

regards, tom lane

Hello

updated patch with documentation

Regards

Pavel

On ons, 2011-11-30 at 10:53 -0500, Tom Lane wrote:
> I think the important point here is that we need to support more than
> one level of validation, and that the higher levels can't really be
> applied by default in CREATE FUNCTION because they may fail on perfectly
> valid code.

How would this work with anything other than PL/pgSQL in practice?

Here is an additional use case: There are a bunch of syntax and style
checkers for Python: pylint, pyflakes, pep8, pychecker, and maybe more.
I would like to have a way to use these for PL/Python. Right now I use
a tool I wrote called plpylint (https://github.com/petere/plpylint),
which pulls the source code out of the database and runs pylint on the
client, which works well enough, but what is being discussed here could
lead to a better solution.

So what I'd like to have is some way to say

check all plpythonu functions [in this schema or whatever] using
checker "pylint"

where "pylint" was previously defined as a checker associated with the
plpythonu language that actually invokes some user-defined function.

Also, what kind of report does this generate?

Tom Lane wrote:
>> Do I understand right that the reason why the check function is
>> different from the validator function is that it would be more
difficult
>> to add the checks to the validator function?
>
>> Is that a good enough argument? From a user's perspective it is
>> difficult to see why some checks are performed at function creation
>> time, while others have to be explicitly checked with CHECK FUNCTION.
>> I think it would be much more intuitive if CHECK FUNCTION does
>> the same as function validation with check_function_bodies on.
>
> I think the important point here is that we need to support more than
> one level of validation, and that the higher levels can't really be
> applied by default in CREATE FUNCTION because they may fail on
perfectly
> valid code.

I understand now.

There are three levels of checking:
1) Validation with check_function_bodies = off (checks nothing).
2) Validation with check_function_bodies = on (checks syntax).
3) CHECK FUNCTION (checks RAISE and objects referenced in the function).

As long as 3) implies 2) (which I think it does), that makes sense.

I guess I was led astray by the documentation in plhandler.sgml:

Validator functions should typically honor the check_function_bodies
parameter: [...] this parameter is turned off by pg_dump so that it
can load procedural language functions without worrying about possible
dependencies of the function bodies on other database objects.

"Dependencyies on other database objects" seems more like a description
of CHECK FUNCTION.
But I guess that this documentation should be changed anyway to describe
the check function.

> A bigger issue is that once you think about more than one kind of
check,
> it becomes apparent that we might need some user-specifiable options
to
> control which checks are applied. And I see no provision for that
here.

My attempt at a syntax that could also cover Peter's wish for multiple
checker functions:

CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])

Yours,
Laurenz Albe

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On ons, 2011-11-30 at 10:53 -0500, Tom Lane wrote:
>> I think the important point here is that we need to support more than
>> one level of validation, and that the higher levels can't really be
>> applied by default in CREATE FUNCTION because they may fail on perfectly
>> valid code.

> How would this work with anything other than PL/pgSQL in practice?

Well, that's TBD by the individual PL authors, but it hardly seems
implausible that there might be lint-like checks applicable in many
PLs. As long as we have the functionality pushed out to a PL-specific
checker function, the details can be worked out later.

> So what I'd like to have is some way to say

> check all plpythonu functions [in this schema or whatever] using
> checker "pylint"

> where "pylint" was previously defined as a checker associated with the
> plpythonu language that actually invokes some user-defined function.

That sounds like a language-specific option to me.

> Also, what kind of report does this generate?

Good question. I suspect what Pavel has now will raise errors, but that
doesn't scale very nicely to checking more than one function, or even to
finding more than one bug in a single function.

My first instinct is to say that it should work like plain EXPLAIN, ie,
deliver a textual report that we send as if it were a query result.

regards, tom lane

Hello
>
>> Also, what kind of report does this generate?
>
> Good question.  I suspect what Pavel has now will raise errors, but that
> doesn't scale very nicely to checking more than one function, or even to
> finding more than one bug in a single function.
>

I stop on first error now. Reason is reuse of functionality that can
to mark a critical point (bug) of embedded query in console.

Continuous checking is possible (plpgsql), but there are a few
critical bugs, that does stop. For example - any buggy assign to
record var causes uninitialized variable and any later checks are
affected. This is possible when ASSIGN, FOR SELECT, SELECT INTO
statements are used. It is small part of possible bugs but relative
often pattern. So I didn't worry about it yet.

> My first instinct is to say that it should work like plain EXPLAIN, ie,
> deliver a textual report that we send as if it were a query result.
>

can be - but EXPLAIN raises exception too, when there some error.
There should be a some possibility to simply identify result. I am not
sure if checking on empty result is good way. A raising exception
should be option. When we return text, then we have to think about
some structured form result - line, position, message. A advance of
exception on first issue is fact, so these questions are solved.

so CHECK can returns lines - like EXPLAIN, but can be nice and helpful
for this moment a GUC - some like check_raises_exception = on|off.
Default should be "off". And I have to think about some FORMAT option.

is it good plan?

Pavel

>                        regards, tom lane

Hello

>
> My attempt at a syntax that could also cover Peter's wish for multiple
> checker functions:
>
> CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
>  [ USING check_function ] OPTIONS (optname optarg [, ...])
>

check_function should be related to one language, so you have to
specify language if you would to specify check_function (if we would
to have more check functions for one language).

Regards

Pavel Stehule

2011/12/2 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
>>
>> My attempt at a syntax that could also cover Peter's wish for multiple
>> checker functions:
>>
>> CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
>>  [ USING check_function ] OPTIONS (optname optarg [, ...])
>>
>

some other idea about other using CHECK FUNCTION

CHECK FUNCTION func(args)
RETURNS ... AS $$

$$ LANGUAGE xxx

This should to do check of function body without affect on registered
function. This is addition to previous defined syntax.

Nice a day

Pavel

Pavel Stehule wrote:
>> My attempt at a syntax that could also cover Peter's wish for multiple
>> checker functions:
>>
>> CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
>>  [ USING check_function ] OPTIONS (optname optarg [, ...])

> check_function should be related to one language, so you have to
> specify language if you would to specify check_function (if we would
> to have more check functions for one language).

Right, I forgot LANGUAGE:

CHECK FUNCTION { func(args) | ALL IN LANGUAGE pl [IN SCHEMA schema] [FOR ROLE user] }
[ USING check_function ] OPTIONS (optname optarg [, ...])

If func(args) is given, the language can be inferred.

Yours,
Laurenz Albe

Hello

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.

postgres=# check function all in schema public;
NOTICE: skip check function "hello()", it use C or internal language
NOTICE: skip check function "tri()", it is trigger function
NOTICE: skip check function "vloz_do_foo(integer)", language "sql"
hasn't checker function
NOTICE: skip check function "mojefunc(integer)", language "sql"
hasn't checker function
NOTICE: skip check function "myleft(text,integer)", language "sql"
hasn't checker function
NOTICE: checked function "array_random(integer,integer)"
NOTICE: skip check function "array_random1(integer,integer)",
language "sql" hasn't checker function
NOTICE: checked function "vrattab(integer)"
NOTICE: checked function "yyy()"
NOTICE: checked function "xxx()"
WARNING: error in function "zpracuj(integer)"
LINE 1: select h from hodnoty
^
DETAIL: column "h" does not exist
QUERY: select h from hodnoty
CONTEXT: line 4 at FOR over SELECT rows
WARNING: error in function "ii(integer)"
LINE 1: SELECT (select aa from i)
^
DETAIL: column "aa" does not exist
QUERY: SELECT (select aa from i)
CONTEXT: line 3 at RETURN
CHECK FUNCTION

postgres=# check function all in schema public in language plpgsql;
NOTICE: skip check function "tri()", it is trigger function
NOTICE: checked function "array_random(integer,integer)"
NOTICE: checked function "vrattab(integer)"
NOTICE: checked function "yyy()"
NOTICE: checked function "xxx()"
WARNING: error in function "zpracuj(integer)"
LINE 1: select h from hodnoty
^
DETAIL: column "h" does not exist
QUERY: select h from hodnoty
CONTEXT: line 4 at FOR over SELECT rows
WARNING: error in function "ii(integer)"
LINE 1: SELECT (select aa from i)
^
DETAIL: column "aa" does not exist
QUERY: SELECT (select aa from i)
CONTEXT: line 3 at RETURN
CHECK FUNCTION

postgres=# check function all in schema public in language plpgsql for role www;
NOTICE: nothing to check
CHECK FUNCTION

please, try it

Regards

Pavel Stehule

2011/12/3 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>>> My attempt at a syntax that could also cover Peter's wish for multiple
>>> checker functions:
>>>
>>> CHECK FUNCTION { func(args) | ALL [IN SCHEMA schema] [FOR ROLE user] }
>>>  [ USING check_function ] OPTIONS (optname optarg [, ...])
>
>> check_function should be related to one language, so you have to
>> specify language if you would to specify check_function (if we would
>> to have more check functions for one language).
>
> Right, I forgot LANGUAGE:
>
> CHECK FUNCTION { func(args) | ALL IN LANGUAGE pl [IN SCHEMA schema] [FOR ROLE user] }
>  [ USING check_function ] OPTIONS (optname optarg [, ...])
>
> If func(args) is given, the language can be inferred.
>
> Yours,
> Laurenz Albe

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.

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.

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).

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".

Yours,
Laurenz Albe

2011/12/7 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)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

Pavel Stehule wrote:
>> 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.

I see.

>> 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

I'll give it a try.

Yours,
Laurenz Albe

Hello

updated version

changes:

* CHECK FUNCTION ALL; is enabled - in this case functions from
pg_catalog schema are ignored

I looked on parser, and I didn't other changes there - IN SCHEMA, FOR
ROLE are used more time there, so our usage will be consistent

Regards

Pavel

2011/12/7 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>>> 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.
>
> I see.
>
>>> 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
>
> I'll give it a try.
>
> Yours,
> Laurenz Albe

Hello

a small addition

* don't check SQL functions - are checked well now
* don't check functions from information_schema too

Regards

Pavel

2011/12/8 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> updated version
>
> changes:
>
> * CHECK FUNCTION ALL; is enabled - in this case functions from
> pg_catalog schema are ignored
>
> I looked on parser, and I didn't other changes there - IN SCHEMA, FOR
> ROLE are used more time there, so our usage will be consistent
>
> Regards
>
> Pavel
>
> 2011/12/7 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>> Pavel Stehule wrote:
>>>> 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.
>>
>> I see.
>>
>>>> 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
>>
>> I'll give it a try.
>>
>> Yours,
>> Laurenz Albe

Pavel Stehule wrote:
> updated version
>
> changes:
>
> * CHECK FUNCTION ALL; is enabled - in this case functions from
> pg_catalog schema are ignored
>
> I looked on parser, and I didn't other changes there - IN SCHEMA, FOR
> ROLE are used more time there, so our usage will be consistent

> a small addition
>
> * don't check SQL functions - are checked well now
> * don't check functions from information_schema too

One hunk in the patch fails due to conflict with
commit d5f23af6bfbc454e86dd16e5c7a0bfc0cf6189d0
(Peter Eisentraut's const patch).

There are also compiler warnings about discarded const
qualifiers in backend/nodes/copyfuncs.c,
backend/nodes/equalfuncs.c and backend/parser/gram.y.

There is a bug when ALL IN SCHEMA or ALL IN LANGUAGE
is used:

test=> CHECK FUNCTION ALL IN LANGUAGE plpgsql;
ERROR: language "language" does not exist
test=> CHECK FUNCTION ALL IN SCHEMA laurenz;
ERROR: schema "schema" does not exist

Something gets mixed up here.

I like the idea that CHECK FUNCTION ALL without additional
clauses works and ignores pg_catalog and information_schema!

I'm working on some documentation, but it won't be final before
the functionality is agreed upon.

Yours,
Laurenz Albe

Hello

there is fixed version

Regards

Pavel

2011/12/9 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> updated version
>>
>> changes:
>>
>> * CHECK FUNCTION ALL; is enabled - in this case functions from
>> pg_catalog schema are ignored
>>
>> I looked on parser, and I didn't other changes there - IN SCHEMA, FOR
>> ROLE are used more time there, so our usage will be consistent
>
>> a small addition
>>
>> * don't check SQL functions - are checked well now
>> * don't check functions from information_schema too
>
> One hunk in the patch fails due to conflict with
> commit d5f23af6bfbc454e86dd16e5c7a0bfc0cf6189d0
> (Peter Eisentraut's const patch).
>
> There are also compiler warnings about discarded const
> qualifiers in backend/nodes/copyfuncs.c,
> backend/nodes/equalfuncs.c and backend/parser/gram.y.
>
> There is a bug when ALL IN SCHEMA or ALL IN LANGUAGE
> is used:
>
> test=> CHECK FUNCTION ALL IN LANGUAGE plpgsql;
> ERROR:  language "language" does not exist
> test=> CHECK FUNCTION ALL IN SCHEMA laurenz;
> ERROR:  schema "schema" does not exist
>
> Something gets mixed up here.
>
> I like the idea that CHECK FUNCTION ALL without additional
> clauses works and ignores pg_catalog and information_schema!
>
> I'm working on some documentation, but it won't be final before
> the functionality is agreed upon.
>
> Yours,
> Laurenz Albe

Pavel Stehule wrote:
> there is fixed version

Here is my attempt at a doc patch.

Could you add it to your patch so that all is in a single patch?

Yours,
Laurenz Albe

Hello

2011/12/9 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> there is fixed version
>
> Here is my attempt at a doc patch.
>
> Could you add it to your patch so that all is in a single patch?
>

there is merged patch

Thank you

Regards

Pavel

> Yours,
> Laurenz Albe

Pavel Stehule wrote:
> there is merged patch

Works fine, except that there are still missing const qualifiers
in copyfuncs.c and equalfuncs.c that lead to compiler warnings.

One thing I forgot to mention:
I thought there was a consensus to add a WITH() or OPTIONS() clause
to pass options to the checker function:
http://archives.postgresql.org/message-id/12568.1322669638@sss.pgh.pa.us

I think this should be there so that the API does not have to be
changed in the future.

Yours,
Laurenz Albe

hello

2011/12/12 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> there is merged patch
>
> Works fine, except that there are still missing const qualifiers
> in copyfuncs.c and equalfuncs.c that lead to compiler warnings.
>
> One thing I forgot to mention:
> I thought there was a consensus to add a WITH() or OPTIONS() clause
> to pass options to the checker function:
> http://archives.postgresql.org/message-id/12568.1322669638@sss.pgh.pa.us
>
> I think this should be there so that the API does not have to be
> changed in the future.
>

there is just one question - how propagate options to check functions

I am thinking about third parameter - probably text array

??
Regards

Pavel

> Yours,
> Laurenz Albe

Pavel Stehule wrote:
>> One thing I forgot to mention:
>> I thought there was a consensus to add a WITH() or OPTIONS() clause
>> to pass options to the checker function:
>> http://archives.postgresql.org/message-id/12568.1322669638@sss.pgh.pa.us
>>
>> I think this should be there so that the API does not have to be
>> changed in the future.
>>

> there is just one question - how propagate options to check functions
>
> I am thinking about third parameter - probably text array

Either that, or couldn't you pass an option List as data type "internal"?

I don't know what is most natural or convenient.

Yours,
Laurenz Albe

2011/12/13 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>>> One thing I forgot to mention:
>>> I thought there was a consensus to add a WITH() or OPTIONS() clause
>>> to pass options to the checker function:
>>> http://archives.postgresql.org/message-id/12568.1322669638@sss.pgh.pa.us
>>>
>>> I think this should be there so that the API does not have to be
>>> changed in the future.
>>>
>
>> there is just one question - how propagate options to check functions
>>
>> I am thinking about third parameter - probably text array
>
> Either that, or couldn't you pass an option List as data type "internal"?
>

this is question - internal is most simply solution, but then we
cannot to call check function directly

Regards

Pavel

> I don't know what is most natural or convenient.
>
> Yours,
> Laurenz Albe

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/12/13 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>> Either that, or couldn't you pass an option List as data type "internal"?

> this is question - internal is most simply solution, but then we
> cannot to call check function directly

Yeah, one of the proposals for allowing people to specify complicated
conditions about what to check was to tell them to do
select checker(oid) from pg_proc where any-random-condition;
If the checker isn't user-callable then we lose that escape hatch, and
the only selection conditions that will ever be possible are the ones
we take the trouble to shoehorn into the CHECK FUNCTION statement.
Doesn't seem like a good thing to me.

regards, tom lane

2011/12/13 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2011/12/13 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
>>> Either that, or couldn't you pass an option List as data type "internal"?
>
>> this is question - internal is most simply solution, but then we
>> cannot to call check function directly
>
> Yeah, one of the proposals for allowing people to specify complicated
> conditions about what to check was to tell them to do
>        select checker(oid) from pg_proc where any-random-condition;

> If the checker isn't user-callable then we lose that escape hatch, and
> the only selection conditions that will ever be possible are the ones
> we take the trouble to shoehorn into the CHECK FUNCTION statement.
> Doesn't seem like a good thing to me.

yes, it is reason why I thinking just about string array.

I have not idea about other PL, but options for plpgsql can be one
word and checker function can simply parse two or more words options.

Now I would to implement flags "quite" - ignore NOTIFY messages and
"fatal_errors" to stop on first error.

Regards

Pavel

>
>                        regards, tom lane

Hello

2011/12/12 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> there is merged patch
>
> Works fine, except that there are still missing const qualifiers
> in copyfuncs.c and equalfuncs.c that lead to compiler warnings.
>
> One thing I forgot to mention:
> I thought there was a consensus to add a WITH() or OPTIONS() clause
> to pass options to the checker function:
> http://archives.postgresql.org/message-id/12568.1322669638@sss.pgh.pa.us
>
> I think this should be there so that the API does not have to be
> changed in the future.
>

changes:

* fixed warnings
* support for options - actually only two options are supported -
quite and fatal_errors

these options are +/- useful - main reason for their existence is
testing of support of options - processing on CHECK ... stmt side and
processing on checker function side.

options are send as 2d text array - some like
'{{quite,on},{fatal_errors,on}} - so direct call of checker function
is possible

* regress test for multi check

Regards

Pavel

> Yours,
> Laurenz Albe

Pavel Stehule wrote:
> changes:
>
> * fixed warnings
> * support for options - actually only two options are supported -
> quite and fatal_errors
>
> these options are +/- useful - main reason for their existence is
> testing of support of options - processing on CHECK ... stmt side and
> processing on checker function side.
>
> options are send as 2d text array - some like
> '{{quite,on},{fatal_errors,on}} - so direct call of checker function
> is possible
>
> * regress test for multi check

First of all: It should be "quiet" and not "quite".

The patch applies and builds fine.

It fails one of ist own regression tests, here is the diff:

*** /postgres/cvs/postgresql/src/test/regress/expected/plpgsql.out 2011-12-14 11:50:44.000000000 +0100
--- /postgres/cvs/postgresql/src/test/regress/results/plpgsql.out 2011-12-14 16:19:45.000000000 +0100
***************
*** 4975,4991 ****
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
- NOTICE: checked function "plpgsql_check.fce1()"
WARNING: error in function "plpgsql_check.fce2()"
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function "plpgsql_check.fce3()"
check function all in schema plpgsql_check with (quite);
WARNING: error in function "plpgsql_check.fce2()"
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
- NOTICE: checked function "plpgsql_check.fce1()"
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function "fce2" line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);
--- 4975,4990 ----
end;
$$ language plpgsql;
check function all in schema plpgsql_check;
WARNING: error in function "plpgsql_check.fce2()"
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
NOTICE: checked function "plpgsql_check.fce3()"
+ NOTICE: checked function "plpgsql_check.fce1()"
check function all in schema plpgsql_check with (quite);
WARNING: error in function "plpgsql_check.fce2()"
DETAIL: too few parameters specified for RAISE
CONTEXT: line 3 at RAISE
check function all in schema plpgsql_check with (fatal_errors);
ERROR: too few parameters specified for RAISE
CONTEXT: PL/pgSQL function "fce2" line 3 at RAISE
check function all in schema plpgsql_check with (quite, fatal_errors on);

======================================================================

The "quiet" option is not very intuitive:

test=> CHECK FUNCTION ALL WITH (quite 'off');
NOTICE: skip check function "atrig()", it is trigger function
NOTICE: skip check function "perl_max(integer,integer)", language "plperl" hasn't checker function
NOTICE: checked function "ok()"
NOTICE: checked function "newstyle(integer)"
CHECK FUNCTION

test=> CHECK FUNCTION ALL WITH (quite 'on');
NOTICE: skip check function "atrig()", it is trigger function
CHECK FUNCTION

I understand that "quiet" cannot silence this message, nor
"skip ..., uses C language" and "skip ..., it uses internal language",
but that means that it is not very useful as it is.

If all we need is a sample option, I think that "fatal_errors" is
enough, and I think that is an option that can be useful.

Yours,
Laurenz Albe

Hello

so removed "quite" option
and removed multiple check regression tests also - there is missing
explicit order of function checking, so regress tests can fail :(

Regards

Pavel

2011/12/14 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> changes:
>>
>> * fixed warnings
>> * support for options - actually only two options are supported -
>> quite and fatal_errors
>>
>> these options are +/- useful - main reason for their existence is
>> testing of  support of options - processing on CHECK ... stmt side and
>> processing on checker function side.
>>
>> options are send as 2d text array - some like
>> '{{quite,on},{fatal_errors,on}} - so direct call of checker function
>> is possible
>>
>> * regress test for multi check
>
> First of all: It should be "quiet" and not "quite".
>
> The patch applies and builds fine.
>
> It fails one of ist own regression tests, here is the diff:
>
> *** /postgres/cvs/postgresql/src/test/regress/expected/plpgsql.out      2011-12-14 11:50:44.000000000 +0100
> --- /postgres/cvs/postgresql/src/test/regress/results/plpgsql.out       2011-12-14 16:19:45.000000000 +0100
> ***************
> *** 4975,4991 ****
>  end;
>  $$ language plpgsql;
>  check function all in schema plpgsql_check;
> - NOTICE:  checked function "plpgsql_check.fce1()"
>  WARNING:  error in function "plpgsql_check.fce2()"
>  DETAIL:  too few parameters specified for RAISE
>  CONTEXT:  line 3 at RAISE
>  NOTICE:  checked function "plpgsql_check.fce3()"
>  check function all in schema plpgsql_check with (quite);
>  WARNING:  error in function "plpgsql_check.fce2()"
>  DETAIL:  too few parameters specified for RAISE
>  CONTEXT:  line 3 at RAISE
>  check function all in schema plpgsql_check with (fatal_errors);
> - NOTICE:  checked function "plpgsql_check.fce1()"
>  ERROR:  too few parameters specified for RAISE
>  CONTEXT:  PL/pgSQL function "fce2" line 3 at RAISE
>  check function all in schema plpgsql_check with (quite, fatal_errors on);
> --- 4975,4990 ----
>  end;
>  $$ language plpgsql;
>  check function all in schema plpgsql_check;
>  WARNING:  error in function "plpgsql_check.fce2()"
>  DETAIL:  too few parameters specified for RAISE
>  CONTEXT:  line 3 at RAISE
>  NOTICE:  checked function "plpgsql_check.fce3()"
> + NOTICE:  checked function "plpgsql_check.fce1()"
>  check function all in schema plpgsql_check with (quite);
>  WARNING:  error in function "plpgsql_check.fce2()"
>  DETAIL:  too few parameters specified for RAISE
>  CONTEXT:  line 3 at RAISE
>  check function all in schema plpgsql_check with (fatal_errors);
>  ERROR:  too few parameters specified for RAISE
>  CONTEXT:  PL/pgSQL function "fce2" line 3 at RAISE
>  check function all in schema plpgsql_check with (quite, fatal_errors on);
>
> ======================================================================
>
> The "quiet" option is not very intuitive:
>
> test=> CHECK FUNCTION ALL WITH (quite 'off');
> NOTICE:  skip check function "atrig()", it is trigger function
> NOTICE:  skip check function "perl_max(integer,integer)", language "plperl" hasn't checker function
> NOTICE:  checked function "ok()"
> NOTICE:  checked function "newstyle(integer)"
> CHECK FUNCTION
>
> test=> CHECK FUNCTION ALL WITH (quite 'on');
> NOTICE:  skip check function "atrig()", it is trigger function
> CHECK FUNCTION
>
> I understand that "quiet" cannot silence this message, nor
> "skip ..., uses C language" and "skip ..., it uses internal language",
> but that means that it is not very useful as it is.
>
> If all we need is a sample option, I think that "fatal_errors" is
> enough, and I think that is an option that can be useful.
>
> Yours,
> Laurenz Albe

Pavel Stehule wrote:
> so removed "quite" option
> and removed multiple check regression tests also - there is missing
> explicit order of function checking, so regress tests can fail :(

There seems to be a problem with the SET clause of CREATE FUNCTION:

ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END';
CREATE FUNCTION
ftest=# CHECK FUNCTION a(integer);
NOTICE: checked function "a(integer)"
CHECK FUNCTION
ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END';
CREATE FUNCTION
ftest=# CHECK FUNCTION a(integer);
The connection to the server was lost. Attempting reset: Failed.
!>

Yours,
Laurenz Albe

Hello

2011/12/15 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> so removed "quite" option
>> and removed multiple check regression tests also - there is missing
>> explicit order of function checking, so regress tests can fail :(
>
> There seems to be a problem with the SET clause of CREATE FUNCTION:
>
> ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
>        LANGUAGE plpgsql AS 'BEGIN RETURN 2*$1; END';
> CREATE FUNCTION
> ftest=# CHECK FUNCTION a(integer);
> NOTICE:  checked function "a(integer)"
> CHECK FUNCTION
> ftest=# CREATE OR REPLACE FUNCTION a(integer) RETURNS integer
>        LANGUAGE plpgsql SET search_path=public AS 'BEGIN RETURN 2*$1; END';
> CREATE FUNCTION
> ftest=# CHECK FUNCTION a(integer);
> The connection to the server was lost. Attempting reset: Failed.
> !>
>

There was bug - missing detoast call

fixed

Regards

Pavel

> Yours,
> Laurenz Albe

Hello

one small update - better emulation of environment for security
definer functions

Regards

Pavel

Pavel Stehule wrote:
> one small update - better emulation of environment for security
> definer functions

Patch applies and compiles fine, core functionality works fine.

I found a little bug:

In backend/commands/functioncmds.c,
function CheckFunction(CheckFunctionStmt *stmt),
while you perform the table scan for CHECK FUNCTION ALL,
you use the variable funcOid to hold the OID of the current
function in the loop.

If no appropriate function is found in the loop, the
check immediately after the table scan will not succeed
because funcOid holds the OID of the last function scanned
in the loop.
As a consequence, CheckFunctionById is called for this
function.

Here is a demonstration:
test=> CHECK FUNCTION ALL IN SCHEMA pg_catalog;
[...]
NOTICE: skip check function "plpgsql_checker(oid,regclass,text[])", uses C language
NOTICE: skip check function "plperl_call_handler()", uses C language
NOTICE: skip check function "plperl_inline_handler(internal)", uses C language
NOTICE: skip check function "plperl_validator(oid)", uses C language
NOTICE: skip check function "plperl_validator(oid)", language "c" hasn't checker function
CHECK FUNCTION

when it should be:
test=> CHECK FUNCTION ALL IN SCHEMA pg_catalog;
[...]
NOTICE: skip check function "plpgsql_checker(oid,regclass,text[])", uses C language
NOTICE: skip check function "plperl_call_handler()", uses C language
NOTICE: skip check function "plperl_inline_handler(internal)", uses C language
NOTICE: skip check function "plperl_validator(oid)", uses C language
NOTICE: nothing to check
CHECK FUNCTION

Another thing struck me as odd:

You have the option "fatal_errors" for the checker function, but you
special case it in CheckFunction(CheckFunctionStmt *stmt) and turn
errors to warnings if it is not set.

Wouldn't it be better to have the checker function ereport a WARNING
or an ERROR depending on the setting? Options should be handled by the
checker function.

Yours,
Laurenz Albe

I just poked at this a bit myself to see how the patch looked. There's
just over 4000 lines in the diff. Even though 1/4 of that is tests,
which is itself encouraging, that's still a good sized feature. The
rate at which code here has still been changing regularly here has me
nervous about considering this a commit candidate right now though. It
seems like it still needs a bit more time to have problems squeezed out
still.

Two ideas I was thinking about here:

-If you take a step back and look at where the problem parts of the code
have been recently, are there any new tests or assertions you might add
to try and detect problems like that in the future? I haven't been
following this closely enough to have any suggestions where, and there
is a lot of error checking aimed at logging already; maybe there's
nothing new to chase there.

-Can we find some larger functions you haven't tested this against yet
to throw at it? It seems able to consume all the cases you've
constructed for it; it would be nice to find some brand new ones it's
never seen before to check.

This has made a lot of progress and seems it will be a good commit
candidate for the next CF. I think it justs a bit more time than we
have left in this CommitFest for it right now, particularly given the
size of the patch. I'm turning this one into "returned with feedback",
but as a mediocre pl/pgsql author I'm hoping to see more updates still.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

2011/12/16 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> Pavel Stehule wrote:
>> one small update - better emulation of environment for security
>> definer functions
>
> Patch applies and compiles fine, core functionality works fine.
>
> I found a little bug:
>
> In backend/commands/functioncmds.c,
> function CheckFunction(CheckFunctionStmt *stmt),
> while you perform the table scan for CHECK FUNCTION ALL,
> you use the variable funcOid to hold the OID of the current
> function in the loop.
>
> If no appropriate function is found in the loop, the
> check immediately after the table scan will not succeed
> because funcOid holds the OID of the last function scanned
> in the loop.
> As a consequence, CheckFunctionById is called for this
> function.
>
> Here is a demonstration:
> test=> CHECK FUNCTION ALL IN SCHEMA pg_catalog;
> [...]
> NOTICE:  skip check function "plpgsql_checker(oid,regclass,text[])", uses C language
> NOTICE:  skip check function "plperl_call_handler()", uses C language
> NOTICE:  skip check function "plperl_inline_handler(internal)", uses C language
> NOTICE:  skip check function "plperl_validator(oid)", uses C language
> NOTICE:  skip check function "plperl_validator(oid)", language "c" hasn't checker function
> CHECK FUNCTION
>
> when it should be:
> test=> CHECK FUNCTION ALL IN SCHEMA pg_catalog;
> [...]
> NOTICE:  skip check function "plpgsql_checker(oid,regclass,text[])", uses C language
> NOTICE:  skip check function "plperl_call_handler()", uses C language
> NOTICE:  skip check function "plperl_inline_handler(internal)", uses C language
> NOTICE:  skip check function "plperl_validator(oid)", uses C language
> NOTICE:  nothing to check
> CHECK FUNCTION

I'll fix it
>
>
> Another thing struck me as odd:
>
> You have the option "fatal_errors" for the checker function, but you
> special case it in CheckFunction(CheckFunctionStmt *stmt) and turn
> errors to warnings if it is not set.
>
> Wouldn't it be better to have the checker function ereport a WARNING
> or an ERROR depending on the setting? Options should be handled by the
> checker function.

The behave that I use, is more rubust and there is only a few lines of
code more.

a) It ensure expectable behave for third party checker function -
exception in checker function doesn't break a multi statement check
function
b) It can ensure same format of error message - because it is
transformed on top level

Regards

Pavel

>
> Yours,
> Laurenz Albe

2011/12/16 Greg Smith <greg(at)2ndquadrant(dot)com>:
> I just poked at this a bit myself to see how the patch looked.  There's just
> over 4000 lines in the diff.  Even though 1/4 of that is tests, which is
> itself encouraging, that's still a good sized feature.  The rate at which
> code here has still been changing regularly here has me nervous about
> considering this a commit candidate right now though.  It seems like it
> still needs a bit more time to have problems squeezed out still.
>
> Two ideas I was thinking about here:
>
> -If you take a step back and look at where the problem parts of the code
> have been recently, are there any new tests or assertions you might add to
> try and detect problems like that in the future?  I haven't been following
> this closely enough to have any suggestions where, and there is a lot of
> error checking aimed at logging already; maybe there's nothing new to chase
> there.

last bug was based on wrong untoasting of function parameters - and it
was hang on assertion - so it was ok

I can recheck code where I can add asserts

There is known issue - I cannot to check multi check statement in
regress tests, because results (notices, errors) can be in random
order. We don't sort functions by oid in check_functions_lookup.

>
> -Can we find some larger functions you haven't tested this against yet to
> throw at it?  It seems able to consume all the cases you've constructed for
> it; it would be nice to find some brand new ones it's never seen before to
> check.
>

I use it for checking of my most large plpgsql project - it is about
300KB plpgsql procedures - but this code is not free - and this module
helps to find lot of bugs.

I have plan to check a more functions from regress tests - but these
tests are no bugs - I checked almost all four months ago - dynamic
sql based and temp table based cannot check.

> This has made a lot of progress and seems it will be a good commit candidate
> for the next CF.  I think it justs a bit more time than we have left in this
> CommitFest for it right now, particularly given the size of the patch.  I'm
> turning this one into "returned with feedback", but as a mediocre pl/pgsql
> author I'm hoping to see more updates still.

I'll send update early

Regards

Pavel

>
> --
> Greg Smith   2ndQuadrant US    greg(at)2ndQuadrant(dot)com   Baltimore, MD
> PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Hello

>
> You have the option "fatal_errors" for the checker function, but you
> special case it in CheckFunction(CheckFunctionStmt *stmt) and turn
> errors to warnings if it is not set.
>
> Wouldn't it be better to have the checker function ereport a WARNING
> or an ERROR depending on the setting? Options should be handled by the
> checker function.
>

A would to process fatal_errors out of checker function - just it is
more robust. This flag has not too sense in plpgsql - but can have a
more sense in other languages.

But I'll think again about flags

note about warnings and errors. Warnings are useless on checker
function level, because they are just shown, but they cannot be
trapped.

maybe result based on tuplestore can be better - I have to look on it.

Regards

Pavel

> Yours,
> Laurenz Albe

On 12/17/2011 04:00 PM, Pavel Stehule wrote:
> I use it for checking of my most large plpgsql project - it is about
> 300KB plpgsql procedures - but this code is not free - and this module
> helps to find lot of bugs.

Great. If you continue to check against that regularly, that makes me
feel better. I was guessing you had a large body of such source code
around, and knowing it executed correctly against all of it improves my
confidence here.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

2011/12/19 Greg Smith <greg(at)2ndquadrant(dot)com>:
> On 12/17/2011 04:00 PM, Pavel Stehule wrote:
>>
>> I use it for checking of my most large plpgsql project - it is about
>> 300KB plpgsql procedures - but this code is not free - and this module
>> helps to find lot of bugs.
>
>
> Great.  If you continue to check against that regularly, that makes me feel
> better.  I was guessing you had a large body of such source code around, and
> knowing it executed correctly against all of it improves my confidence here.
>

I am not alone

a subset is used in plpgsql_lint and I know about some commercial
subjects that use it too.

https://github.com/okbob/plpgsql_lint

but code in check function is little newer. It can interesting test
some code that is wroted by person with background from other db,
because they use a different patterns

I don't use a explicit cursors for example - on other hand, I use
exception intensively in my last project. We can ask people from
LedgerSMB about testing if somebody has contact

Regards

Pavel

>
> --
> Greg Smith   2ndQuadrant US    greg(at)2ndQuadrant(dot)com   Baltimore, MD
> PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
>

Hello all

here is new version of CHECK FUNCTION patch

I changed implementation of interface:

* checked functions returns table instead raising exceptions - it
necessary for describing more issues inside one function - and it
allow to use better structured data then ExceptionData

postgres=# select lineno, statement, sqlstate, message, detail, hint,
level, "position", query from plpgsql_checker('f1()', 0, '{}', false);
lineno | statement | sqlstate | message
| detail | hint | level | position | query
--------+---------------+----------+--------------------------------------------+--------+--------+-------+----------+----------------------
4 | SQL statement | 42703 | column "c" of relation "t1" does
not exist | [null] | [null] | error | 15 | update t1 set c = 30
7 | RAISE | 42P01 | missing FROM-clause entry for
table "r" | [null] | [null] | error | 8 | SELECT r.c
7 | RAISE | 42601 | too few parameters specified for
RAISE | [null] | [null] | error | [null] | [null]
(3 rows)

* result of CHECK FUNCTION is simple table (like EXPLAIN - based on
Tom proposition)

postgres=# check function f1();
CHECK FUNCTION
------------------------------------------------------------------------
In function: 'f1()'
error:42703:4:SQL statement:column "c" of relation "t1" does not exist
query:update t1 set c = 30
^
error:42P01:7:RAISE:missing FROM-clause entry for table "r"
query:SELECT r.c
^
error:42601:7:RAISE:too few parameters specified for RAISE
(8 rows)

This change allow a more playing with output

postgres=# check function all in schema public;
CHECK FUNCTION
────────────────────────────────────────────────────────────────────────
In function: 'bubu(integer)'
error:42703:2:assignment:column "v" does not exist
query:SELECT a + v
^
error:42601:3:RETURN:query "SELECT 1,1" returned 2 columns
query:SELECT 1,1

In function: 'f1()'
error:42703:4:SQL statement:column "c" of relation "t1" does not exist
query:update t1 set c = 30
^
error:42P01:7:RAISE:missing FROM-clause entry for table "r"
query:SELECT r.c
^
error:42601:7:RAISE:too few parameters specified for RAISE

Function is valid: 'ff(integer)'
Function is valid: 'fff(integer)'
(18 rows)

Regards

Pavel Stehule

Pavel Stehule wrote:

> here is new version of CHECK FUNCTION patch

I won't be able to review that one because I'll be in
California from Jan 6 to Jan 29.

Yours,
Laurenz Albe

Pavel Stehule wrote:
> here is new version of CHECK FUNCTION patch
>
> I changed implementation of interface:
>
> * checked functions returns table instead raising exceptions - it
> necessary for describing more issues inside one function - and it
> allow to use better structured data then ExceptionData

[...]

> * result of CHECK FUNCTION is simple table (like EXPLAIN - based on
> Tom proposition)

I don't have the time for a complete review, but I tried the patch
and found:

It is in context diff and applies to current master (there is fuzz 1
in one hunk). It contains documentation and regression tests.
Compiles without warnings and passes regression tests.

The two or three CHECK FUNCTIONs I ran worked ok.

The documentation (that I wrote) will need to get updated: currently
it states in two places that the checker function should throw a
warning if it encounters a problem.

Yours,
Laurenz Albe

On 01/01/2012 01:01 PM, Pavel Stehule wrote:
> Hello all
>
> here is new version of CHECK FUNCTION patch

Hi,

I took a shot at reviewing this. The attached version is made against
yesterdays HEAD (which means it applies cleanly) with some updates to
documentation.

> I changed implementation of interface:
>
> * checked functions returns table instead raising exceptions - it
> necessary for describing more issues inside one function - and it
> allow to use better structured data then ExceptionDat

The new interface makes sense to me the way it is, should be usable by
other languages too and for external tools it should enable sufficient
filtering options for what to care about and what not to care about.

Anyway on to full review:

Submission:
Patch has enough documentation (mostly written by Albe Laurenz with some
adjustments from me). It has quite nice set of regression tests which it
passes on both my machines.

Usability:
We certainly want (IMHO it's something we should have had long time ago)
this feature and patch implements it in a way that seems to be useful.
It has pg_dump support.

Feature test:
Works as advertised, passes all regression tests, I tested many real
world functions written by various people and was unable to crash it or
make it misbehave. I can imagine it not working properly with some
EXECUTE statements but I don't believe that is avoidable due to the
nature of PL/pgSQL.

Coding review:
I have no complaints about the code itself, neither did my compiler.
There is no interaction with system so it should not cause any
portability issues.

From my point of view this seems to be ready for committer and barring
any objections I will mark it as such.

Regards
Petr Jelinek (PJMODOS)

I have a few comments about this patch:

I didn't like the fact that the checker calling infrastructure uses
SPI instead of just a FunctionCallN to call the checker function. I
think this should be easily avoidable.

Second, I see that functioncmds.c gets a lot into trigger internals just
to be able to figure out the function starting from a trigger name. I
think it'd be saner to have a new function in trigger.c that returns the
required function OID.

I think CheckFunction would be clearer if the code to check multiple
objects is split out into a separate subroutine.

After CheckFunction there is a leftover function comment without any
following function. There are other spurious hunks that add or remove
single lines too (once in an otherwise untouched file).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

Dne 28. února 2012 17:48 Alvaro Herrera <alvherre(at)commandprompt(dot)com> napsal(a):
>
>
> I have a few comments about this patch:
>
> I didn't like the fact that the checker calling infrastructure uses
> SPI instead of just a FunctionCallN to call the checker function.  I
> think this should be easily avoidable.
>

It is not possible - or it has not simple solution (I don't how to do
it). PLpgSQL_checker is SRF function. SPI is used for processing
returned resultset. I looked to pg source code, and I didn't find any
other pattern than using SPI for SRF function call. It is probably
possible, but it means some code duplication too. I invite any ideas.

> Second, I see that functioncmds.c gets a lot into trigger internals just
> to be able to figure out the function starting from a trigger name.  I
> think it'd be saner to have a new function in trigger.c that returns the
> required function OID.

done

>
> I think CheckFunction would be clearer if the code to check multiple
> objects is split out into a separate subroutine.

done

>
> After CheckFunction there is a leftover function comment without any
> following function.  There are other spurious hunks that add or remove
> single lines too (once in an otherwise untouched file).

fixed

>

I refreshed patch for current git repository.

Regards

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012:

> I refreshed patch for current git repository.

Thanks, I'll have a look.

Oh, another thing -- you shouldn't patch the 1.0 version of the plpgsql
extension. Rather I think you should produce a 1.1 version.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/2/28 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012:
>
>> I refreshed patch for current git repository.
>
> Thanks, I'll have a look.
>
> Oh, another thing -- you shouldn't patch the 1.0 version of the plpgsql
> extension.  Rather I think you should produce a 1.1 version.
>

there is patch with updated tag. I am not sure if there are some
changes in pg_upgrade are necessary??

Regards and thank you

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

In gram.y we have a new check_option_list nonterminal. This is mostly
identical to explain_option_list, except that the option args do not
take a NumericOnly (only opt_boolean_or_string and empty). I wonder if
it's really worthwhile having a bunch of separate productions for this;
how about we just use the existing explain_option_list instead and get
rid of those extra productions?

elog() is used in many user-facing messages (errors and notices). Full
ereport() calls should be used there, so that messages are marked for
translations and so on.

Does the patched pg_dump work with older servers?

I don't like CheckFunction being declared in defrem.h. It seems
completely out of place there. I don't see any better place though, so
I'm thinking maybe we should have a new header file for it (say
commands/functions.h; but we already have executor/functions.h so
perhaps it's better to find another name). This addition means that
there's a distressingly large number of .c files that are now getting
dest.h, which was previously pretty confined.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/2/28 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
>
> In gram.y we have a new check_option_list nonterminal.  This is mostly
> identical to explain_option_list, except that the option args do not
> take a NumericOnly (only opt_boolean_or_string and empty).  I wonder if
> it's really worthwhile having a bunch of separate productions for this;
> how about we just use the existing explain_option_list instead and get
> rid of those extra productions?

I have to look on it

>
> elog() is used in many user-facing messages (errors and notices).  Full
> ereport() calls should be used there, so that messages are marked for
> translations and so on.

yes, I'll fix it

>
> Does the patched pg_dump work with older servers?
>

yes, It should to do

> I don't like CheckFunction being declared in defrem.h.  It seems
> completely out of place there.  I don't see any better place though, so
> I'm thinking maybe we should have a new header file for it (say
> commands/functions.h; but we already have executor/functions.h so
> perhaps it's better to find another name).  This addition means that
> there's a distressingly large number of .c files that are now getting
> dest.h, which was previously pretty confined.
>

you have much better knowledge about headers then me, so, please,
propose solution.

Regards

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/2/28 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
>
> In gram.y we have a new check_option_list nonterminal.  This is mostly
> identical to explain_option_list, except that the option args do not
> take a NumericOnly (only opt_boolean_or_string and empty).  I wonder if
> it's really worthwhile having a bunch of separate productions for this;
> how about we just use the existing explain_option_list instead and get
> rid of those extra productions?
>
> elog() is used in many user-facing messages (errors and notices).  Full
> ereport() calls should be used there, so that messages are marked for
> translations and so on.

I replaced elog by ereport for all not internal errors

>
> Does the patched pg_dump work with older servers?
>

it should to do

> I don't like CheckFunction being declared in defrem.h.  It seems
> completely out of place there.  I don't see any better place though, so
> I'm thinking maybe we should have a new header file for it (say
> commands/functions.h; but we already have executor/functions.h so
> perhaps it's better to find another name).  This addition means that
> there's a distressingly large number of .c files that are now getting
> dest.h, which was previously pretty confined.

please, fix it like you wish

Regards

Pavel

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

I think the way we're passing down the options to the checker is a bit
of a mess. The way it is formulated, it seems to me that we'd need to
add support code in the core CheckFunction for each option we might want
to accept in the PL-specific checkers -- including what type of value
the option receives. As an example, right now we have all but one
option taking a string argument (judging from usage of defGetString());
however, option fatal_errors takes a boolean value, and it converts to
string "on" or "off" which is supposed to be passed down to the checker.

This doesn't seem very future-proof.

(Also, the patch seems to be passing the fatal_errors value twice: first
in the options array, where it is ignored by the plpgsql checker, and a
second time as a separate boolean option. This needs a cleanup).

I don't see any good way to pass down generic options in a generic way.
Maybe we can just state that all option values are going to be passed as
strings -- is that good enough? The other option would be to pass them
using something like pg_node_tree, but then it wouldn't be possible to
call the checker directly instead of through CHECK FUNCTION, which I
think was a requirement. And it'd be a stronger argument against usage
of SPI to call the checker function from CHECK FUNCTION, but that's an
unsolved problem.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/2/29 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> I think the way we're passing down the options to the checker is a bit
> of a mess.  The way it is formulated, it seems to me that we'd need to
> add support code in the core CheckFunction for each option we might want
> to accept in the PL-specific checkers -- including what type of value
> the option receives.  As an example, right now we have all but one
> option taking a string argument (judging from usage of defGetString());
> however, option fatal_errors takes a boolean value, and it converts to
> string "on" or "off" which is supposed to be passed down to the checker.
> This doesn't seem very future-proof.

I don't agree - we has not any datatype that can hold "key/value" list
- and can be used via SQL interface. So two dimensional text array is
simple and generic data type. Theoretically we can use JSON type now,
but I think so array is more generic and better checked then JSON now
(and it require less code - and JSON is still string too). We don't
plan to modify parser to better support of JSON, so text array is more
user friendly. I think so pl checker function can be simply called
with used concept. But I am not against to your proposals. Actually
"concept" of generic options was required in initial discuss and then
there is implemented, but it not used. But cannot be removed, because
probably don't would to change API in next version.

>
> (Also, the patch seems to be passing the fatal_errors value twice: first
> in the options array, where it is ignored by the plpgsql checker, and a
> second time as a separate boolean option.  This needs a cleanup).
>

This is by design. One request for checker function (and check
function statement) was generic support for some optional data. This
can has sense for plperl or plpython, and it are not used now.
Fatal_errors is only proof concept and can be removed. I plan to use
these options in 9.3 for checking of "inline blocks".

> I don't see any good way to pass down generic options in a generic way.
> Maybe we can just state that all option values are going to be passed as
> strings -- is that good enough?  The other option would be to pass them
> using something like pg_node_tree, but then it wouldn't be possible to
> call the checker directly instead of through CHECK FUNCTION, which I
> think was a requirement.  And it'd be a stronger argument against usage
> of SPI to call the checker function from CHECK FUNCTION, but that's an
> unsolved problem.

Using just string needs more complex parsing, and I don't like some
like pg_node_tree too, because it cannot be simple created by hands
for direct call of checker function. Please, accept fact, so we would
to call directly PL checker function - and there all params of this
function should be simple created - and using two dimensional array is
really simple: ARRAY [['source',$$....$$]].

I don't understand why you don't like pass generic options by generic way. Why?

Regards

Pavel Stehule

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Why does CollectCheckedFunctions skip trigger functions? My only guess
is that at one point the checker was not supposed to know how to check
them, and a later version learned about it and this bit wasn't updated;
but maybe there's another reason?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

I've cleaned up the backend code a bit -- see attached. More yet to go
through; I'm mainly sending it out for you (and everyone, really) to
give your opinion on my changes so far.

(I split out the plpgsql checker for the time being into a separate
branch; I'll get on it after I get this part committed.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/3/1 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
>
> Why does CollectCheckedFunctions skip trigger functions?  My only guess
> is that at one point the checker was not supposed to know how to check
> them, and a later version learned about it and this bit wasn't updated;
> but maybe there's another reason?

you cannot to check trigger function without assigned relation -
TupleDescription should be assigned to NEW and OLD variables.

Regards

Pavel

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/3/2 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> I've cleaned up the backend code a bit -- see attached.  More yet to go
> through; I'm mainly sending it out for you (and everyone, really) to
> give your opinion on my changes so far.
>
> (I split out the plpgsql checker for the time being into a separate
> branch; I'll get on it after I get this part committed.)

it looks well

Regards

Pavel Stěhule

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of vie mar 02 05:29:26 -0300 2012:

> you cannot to check trigger function without assigned relation -
> TupleDescription should be assigned to NEW and OLD variables.

Oh, I see, that makes sense.

After mulling over this a bit, I'm dubious about having two separate
commands, one which checks triggers and another that checks non-trigger
functions. Wouldn't it make more sense to have some options into CHECK
FUNCTION so that it receives the trigger and corresponding relation name
to check? For example "check function foo() trigger on tab" or
something like that?

I also wonder if it would make sense to have grammar for "check all
triggers on table xyz" or some such, and even "check all triggers on all
functions".

Another thing is that "CHECK FUNCTION ALL FOR ROLE foo" seems a bit
strange to me. What about "CHECK FUNCTION ALL OWNED BY foo" instead?
("CHECK FUNCTION ALL" seems strange as a whole, but I'm not sure that we
can improve that ... still, if anyone has ideas I'm sure we can discuss)

As a reminder: we also have
CHECK FUNCTION ALL IN SCHEMA f
and
CHECK FUNCTION ALL IN LANGUAGE f
(and combinations thereof)

Thoughts?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/3/2 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Pavel Stehule's message of vie mar 02 05:29:26 -0300 2012:
>
>> you cannot to check trigger function without assigned relation -
>> TupleDescription should be assigned to NEW and OLD variables.
>
> Oh, I see, that makes sense.
>
> After mulling over this a bit, I'm dubious about having two separate
> commands, one which checks triggers and another that checks non-trigger
> functions.  Wouldn't it make more sense to have some options into CHECK
> FUNCTION so that it receives the trigger and corresponding relation name
> to check?  For example "check function foo() trigger on tab" or
> something like that?
>

I don't like it - "check trigger" is simple - and consistent

> I also wonder if it would make sense to have grammar for "check all
> triggers on table xyz" or some such, and even "check all triggers on all
> functions".

this is good idea - and I like it

CHECK TRIGGER ALL ON TABLE X

there are possible some combination like CHECK TRIGGER ALL IN SCHEMA ...;

and similar. But I am not sure, if this is necessary - for some more
complex usage developer can use a direct PL check function.

>
> Another thing is that "CHECK FUNCTION ALL FOR ROLE foo" seems a bit
> strange to me.  What about "CHECK FUNCTION ALL OWNED BY foo" instead?
> ("CHECK FUNCTION ALL" seems strange as a whole, but I'm not sure that we
> can improve that ... still, if anyone has ideas I'm sure we can discuss)

this should not be nice from language view, but it doesn't need new keywords

pattern FOR ROLE, IN SCHEMA are used

ALTER DEFAULT PRIVILEGES FOR ROLE

but OWNED BY is used too (SeqOptElem:)

I agree so OWNED BY sounds better (can be changed)

>
> As a reminder: we also have
> CHECK FUNCTION ALL IN SCHEMA f

it is ok

> and
> CHECK FUNCTION ALL IN LANGUAGE f
> (and combinations thereof)
>

it is ok too

Regards

Pavel

> Thoughts?
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012:
> Hello
>
> Dne 28. února 2012 17:48 Alvaro Herrera <alvherre(at)commandprompt(dot)com> napsal(a):
> >
> >
> > I have a few comments about this patch:
> >
> > I didn't like the fact that the checker calling infrastructure uses
> > SPI instead of just a FunctionCallN to call the checker function.  I
> > think this should be easily avoidable.
>
> It is not possible - or it has not simple solution (I don't how to do
> it). PLpgSQL_checker is SRF function. SPI is used for processing
> returned resultset. I looked to pg source code, and I didn't find any
> other pattern than using SPI for SRF function call. It is probably
> possible, but it means some code duplication too. I invite any ideas.

It wasn't all that difficult -- see below. While at this, I have a
question: how attached you are to the current return format for CHECK
FUNCTION?

check function f1();
CHECK FUNCTION
-------------------------------------------------------------
In function: 'f1()'
error:42804:5:assignment:subscripted object is not an array
(2 rows)

It seems to me that it'd be trivial to make it look like this instead:

check function f1();
function | lineno | statement | sqlstate | message | detail | hint | level | position | query
---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
f1() | 5 | assignment | 42804 | subscripted object is not an array | | | error | |
(1 row)

This looks much nicer to me.

One thing we lose is the caret marking the position of the error -- but
I'm wondering if that really works well. I didn't test it but from the
code it looks to me like it'd misbehave if you had a multiline statement.

Opinions?

/*
* Search and execute the checker function.
*
* returns true, when checked function is valid
*/
static bool
CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
bool fatal_errors, TupOutputState *tstate)
{
HeapTuple tup;
Form_pg_proc proc;
HeapTuple languageTuple;
Form_pg_language lanForm;
Oid languageChecker;
char *funcname;
int result;

tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR, "cache lookup failed for function %u", funcOid);

proc = (Form_pg_proc) GETSTRUCT(tup);

languageTuple = SearchSysCache1(LANGOID, ObjectIdGetDatum(proc->prolang));
Assert(HeapTupleIsValid(languageTuple));

lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
languageChecker = lanForm->lanchecker;

funcname = format_procedure(funcOid);

/* We're all set to call the checker */
if (OidIsValid(languageChecker))
{
TupleDesc tupdesc;
Datum checkret;
FmgrInfo flinfo;
ReturnSetInfo rsinfo;
FunctionCallInfoData fcinfo;

/* create the tuple descriptor that the checker is supposed to return */
tupdesc = CreateTemplateTupleDesc(10, false);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "functionid", REGPROCOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "lineno", INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 3, "statement", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sqlstate", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "message", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 6, "detail", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 7, "hint", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 8, "level", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 9, "position", INT4OID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 10, "query", TEXTOID, -1, 0);

fmgr_info(languageChecker, &flinfo);

rsinfo.type = T_ReturnSetInfo;
rsinfo.econtext = CreateStandaloneExprContext();
rsinfo.expectedDesc = tupdesc;
rsinfo.allowedModes = (int) SFRM_Materialize;
/* returnMode is set by the checker, hopefully ... */
/* isDone is not relevant, since not using ValuePerCall */
rsinfo.setResult = NULL;
rsinfo.setDesc = NULL;

InitFunctionCallInfoData(fcinfo, &flinfo, 4, InvalidOid, NULL, (Node *) &rsinfo);
fcinfo.arg[0] = ObjectIdGetDatum(funcOid);
fcinfo.arg[1] = ObjectIdGetDatum(relid);
fcinfo.arg[2] = PointerGetDatum(options);
fcinfo.arg[3] = BoolGetDatum(fatal_errors);
fcinfo.argnull[0] = false;
fcinfo.argnull[1] = false;
fcinfo.argnull[2] = false;
fcinfo.argnull[3] = false;

checkret = FunctionCallInvoke(&fcinfo);

if (rsinfo.returnMode != SFRM_Materialize)
elog(ERROR, "checker function didn't return a proper return set");
/* XXX we have to do some checking on rsinfo.isDone and checkret here */

if (rsinfo.setResult != NULL)
{
bool isnull;
StringInfoData str;
TupleTableSlot *slot = MakeSingleTupleTableSlot(tupdesc);

initStringInfo(&str);

while (tuplestore_gettupleslot(rsinfo.setResult, true, false, slot))
{
text *message = (text *) DatumGetPointer(slot_getattr(slot, 5, &isnull));

resetStringInfo(&str);

appendStringInfo(&str, "got a message: %s", text_to_cstring(message));
do_text_output_oneline(tstate, str.data);
}

pfree(str.data);
ExecDropSingleTupleTableSlot(slot);
}
}

pfree(funcname);

ReleaseSysCache(languageTuple);
ReleaseSysCache(tup);

return result;
}

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

>
> It wasn't all that difficult -- see below.  While at this, I have a
> question: how attached you are to the current return format for CHECK
> FUNCTION?

TupleDescr is created by language creator. This ensure exactly
expected format, because there are no possible registry check function
with other output tuple descriptor.

>
> check function f1();
>                       CHECK FUNCTION
> -------------------------------------------------------------
>  In function: 'f1()'
>  error:42804:5:assignment:subscripted object is not an array
> (2 rows)
>
> It seems to me that it'd be trivial to make it look like this instead:
>
> check function f1();
> function | lineno | statement  | sqlstate |              message               | detail | hint | level | position | query
> ---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
> f1()     |      5 | assignment | 42804    | subscripted object is not an array |        |      | error |          |
> (1 row)
>
> This looks much nicer to me.

I am strongly disagree.

1. This format is not consistent with other commands,
2. This format is difficult for copy/paste
3. THE ARE NOT CARET - this is really important
5. This form is bad for terminals - there are long rows, and with \x
outout, there are lot of "garbage" for multicommand
4. When you would to this form, you can to directly call SRF PL check
functions.

>
> One thing we lose is the caret marking the position of the error -- but
> I'm wondering if that really works well.  I didn't test it but from the
> code it looks to me like it'd misbehave if you had a multiline statement.
>
> Opinions?

-1

>
>
> /*
>  * Search and execute the checker function.
>  *
>  *   returns true, when checked function is valid
>  */
> static bool
> CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
>                                  bool fatal_errors, TupOutputState *tstate)
> {
>        HeapTuple               tup;
>        Form_pg_proc    proc;
>        HeapTuple               languageTuple;
>        Form_pg_language lanForm;
>        Oid                             languageChecker;
>        char               *funcname;
>        int                             result;
>
>        tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
>        if (!HeapTupleIsValid(tup)) /* should not happen */
>                elog(ERROR, "cache lookup failed for function %u", funcOid);
>
>        proc = (Form_pg_proc) GETSTRUCT(tup);
>
>        languageTuple = SearchSysCache1(LANGOID, ObjectIdGetDatum(proc->prolang));
>        Assert(HeapTupleIsValid(languageTuple));
>
>        lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
>        languageChecker = lanForm->lanchecker;
>
>        funcname = format_procedure(funcOid);
>
>        /* We're all set to call the checker */
>        if (OidIsValid(languageChecker))
>        {
>                TupleDesc               tupdesc;
>                Datum                   checkret;
>                FmgrInfo                flinfo;
>                ReturnSetInfo   rsinfo;
>                FunctionCallInfoData fcinfo;
>
>                /* create the tuple descriptor that the checker is supposed to return */
>                tupdesc = CreateTemplateTupleDesc(10, false);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 1, "functionid", REGPROCOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 2, "lineno", INT4OID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 3, "statement", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sqlstate", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 5, "message", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 6, "detail", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 7, "hint", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 8, "level", TEXTOID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 9, "position", INT4OID, -1, 0);
>                TupleDescInitEntry(tupdesc, (AttrNumber) 10, "query", TEXTOID, -1, 0);
>
>                fmgr_info(languageChecker, &flinfo);
>
>                rsinfo.type = T_ReturnSetInfo;
>                rsinfo.econtext = CreateStandaloneExprContext();
>                rsinfo.expectedDesc = tupdesc;
>                rsinfo.allowedModes = (int) SFRM_Materialize;
>                /* returnMode is set by the checker, hopefully ... */
>                /* isDone is not relevant, since not using ValuePerCall */
>                rsinfo.setResult = NULL;
>                rsinfo.setDesc = NULL;
>
>                InitFunctionCallInfoData(fcinfo, &flinfo, 4, InvalidOid, NULL, (Node *) &rsinfo);
>                fcinfo.arg[0] = ObjectIdGetDatum(funcOid);
>                fcinfo.arg[1] = ObjectIdGetDatum(relid);
>                fcinfo.arg[2] = PointerGetDatum(options);
>                fcinfo.arg[3] = BoolGetDatum(fatal_errors);
>                fcinfo.argnull[0] = false;
>                fcinfo.argnull[1] = false;
>                fcinfo.argnull[2] = false;
>                fcinfo.argnull[3] = false;
>
>                checkret = FunctionCallInvoke(&fcinfo);
>
>                if (rsinfo.returnMode != SFRM_Materialize)
>                        elog(ERROR, "checker function didn't return a proper return set");
>                /* XXX we have to do some checking on rsinfo.isDone and checkret here */
>
>                if (rsinfo.setResult != NULL)
>                {
>                        bool    isnull;
>                        StringInfoData  str;
>                        TupleTableSlot  *slot = MakeSingleTupleTableSlot(tupdesc);
>
>                        initStringInfo(&str);
>
>                        while (tuplestore_gettupleslot(rsinfo.setResult, true, false, slot))
>                        {
>                                text *message = (text *) DatumGetPointer(slot_getattr(slot, 5, &isnull));
>
>                                resetStringInfo(&str);
>
>                                appendStringInfo(&str, "got a message: %s", text_to_cstring(message));
>                                do_text_output_oneline(tstate, str.data);
>                        }
>
>                        pfree(str.data);
>                        ExecDropSingleTupleTableSlot(slot);
>                }
>        }
>
>        pfree(funcname);
>
>        ReleaseSysCache(languageTuple);
>        ReleaseSysCache(tup);
>
>        return result;
> }
>

Without correct registration you cannot to call PL check function
directly simply. I don't thing so this is good price for removing a
few SPI lines. I don't understand why you don't like SPI. It is used
more times in code for similar purpose.

so from me -1

Regards

Pavel Stehule

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/3/3 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
>>
>> It wasn't all that difficult -- see below.  While at this, I have a
>> question: how attached you are to the current return format for CHECK
>> FUNCTION?
>
> TupleDescr is created by language creator. This ensure exactly
> expected format, because there are no possible registry check function
> with other output tuple descriptor.
>
>>
>> check function f1();
>>                       CHECK FUNCTION
>> -------------------------------------------------------------
>>  In function: 'f1()'
>>  error:42804:5:assignment:subscripted object is not an array
>> (2 rows)
>>
>> It seems to me that it'd be trivial to make it look like this instead:
>>
>> check function f1();
>> function | lineno | statement  | sqlstate |              message               | detail | hint | level | position | query
>> ---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
>> f1()     |      5 | assignment | 42804    | subscripted object is not an array |        |      | error |          |
>> (1 row)
>>
>> This looks much nicer to me.

This was similar to first design - it is near to result of direct PL
check function call. But Tom and Albe had different opinion - check a
thread three months ago, and I had to agree with them - they proposed
better behave for using in psql console - and result is more similar
to usual output when exception is raised.

>
> I am strongly disagree.
>
> 1. This format is not consistent with other commands,
> 2. This format is difficult for copy/paste
> 3. THE ARE NOT CARET - this is really important
> 5. This form is bad for terminals - there are long rows, and with \x
> outout, there are lot of "garbage" for multicommand
> 4. When you would to this form, you can to directly call SRF PL check
> functions.
>
>>
>> One thing we lose is the caret marking the position of the error -- but
>> I'm wondering if that really works well.  I didn't test it but from the
>> code it looks to me like it'd misbehave if you had a multiline statement.
>>
>> Opinions?
>
>
>
>>
>>
>> /*
>>  * Search and execute the checker function.
>>  *
>>  *   returns true, when checked function is valid
>>  */
>> static bool
>> CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,
>>                                  bool fatal_errors, TupOutputState *tstate)
>> {
>>        HeapTuple               tup;
>>        Form_pg_proc    proc;
>>        HeapTuple               languageTuple;
>>        Form_pg_language lanForm;
>>        Oid                             languageChecker;
>>        char               *funcname;
>>        int                             result;
>>
>>        tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));
>>        if (!HeapTupleIsValid(tup)) /* should not happen */
>>                elog(ERROR, "cache lookup failed for function %u", funcOid);
>>
>>        proc = (Form_pg_proc) GETSTRUCT(tup);
>>
>>        languageTuple = SearchSysCache1(LANGOID, ObjectIdGetDatum(proc->prolang));
>>        Assert(HeapTupleIsValid(languageTuple));
>>
>>        lanForm = (Form_pg_language) GETSTRUCT(languageTuple);
>>        languageChecker = lanForm->lanchecker;
>>
>>        funcname = format_procedure(funcOid);
>>
>>        /* We're all set to call the checker */
>>        if (OidIsValid(languageChecker))
>>        {
>>                TupleDesc               tupdesc;
>>                Datum                   checkret;
>>                FmgrInfo                flinfo;
>>                ReturnSetInfo   rsinfo;
>>                FunctionCallInfoData fcinfo;
>>
>>                /* create the tuple descriptor that the checker is supposed to return */
>>                tupdesc = CreateTemplateTupleDesc(10, false);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 1, "functionid", REGPROCOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 2, "lineno", INT4OID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 3, "statement", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sqlstate", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 5, "message", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 6, "detail", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 7, "hint", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 8, "level", TEXTOID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 9, "position", INT4OID, -1, 0);
>>                TupleDescInitEntry(tupdesc, (AttrNumber) 10, "query", TEXTOID, -1, 0);
>>
>>                fmgr_info(languageChecker, &flinfo);
>>
>>                rsinfo.type = T_ReturnSetInfo;
>>                rsinfo.econtext = CreateStandaloneExprContext();
>>                rsinfo.expectedDesc = tupdesc;
>>                rsinfo.allowedModes = (int) SFRM_Materialize;
>>                /* returnMode is set by the checker, hopefully ... */
>>                /* isDone is not relevant, since not using ValuePerCall */
>>                rsinfo.setResult = NULL;
>>                rsinfo.setDesc = NULL;
>>
>>                InitFunctionCallInfoData(fcinfo, &flinfo, 4, InvalidOid, NULL, (Node *) &rsinfo);
>>                fcinfo.arg[0] = ObjectIdGetDatum(funcOid);
>>                fcinfo.arg[1] = ObjectIdGetDatum(relid);
>>                fcinfo.arg[2] = PointerGetDatum(options);
>>                fcinfo.arg[3] = BoolGetDatum(fatal_errors);
>>                fcinfo.argnull[0] = false;
>>                fcinfo.argnull[1] = false;
>>                fcinfo.argnull[2] = false;
>>                fcinfo.argnull[3] = false;
>>
>>                checkret = FunctionCallInvoke(&fcinfo);
>>
>>                if (rsinfo.returnMode != SFRM_Materialize)
>>                        elog(ERROR, "checker function didn't return a proper return set");
>>                /* XXX we have to do some checking on rsinfo.isDone and checkret here */
>>
>>                if (rsinfo.setResult != NULL)
>>                {
>>                        bool    isnull;
>>                        StringInfoData  str;
>>                        TupleTableSlot  *slot = MakeSingleTupleTableSlot(tupdesc);
>>
>>                        initStringInfo(&str);
>>
>>                        while (tuplestore_gettupleslot(rsinfo.setResult, true, false, slot))
>>                        {
>>                                text *message = (text *) DatumGetPointer(slot_getattr(slot, 5, &isnull));
>>
>>                                resetStringInfo(&str);
>>
>>                                appendStringInfo(&str, "got a message: %s", text_to_cstring(message));
>>                                do_text_output_oneline(tstate, str.data);
>>                        }
>>
>>                        pfree(str.data);
>>                        ExecDropSingleTupleTableSlot(slot);
>>                }
>>        }
>>
>>        pfree(funcname);
>>
>>        ReleaseSysCache(languageTuple);
>>        ReleaseSysCache(tup);
>>
>>        return result;
>> }
>>
>
> Without correct registration you cannot to call PL check function
> directly simply. I don't thing so this is good price for removing a
> few SPI lines. I don't understand why you don't like SPI. It is used
> more times in code for similar purpose.
>
> so from me -1

this disallow direct PL check function call - so any more complex
situation cannot be solved by SQL, but must be solved by PL/pgSQL with
dynamic SQL

so I don't like it. We can talk about format for check_options - but I
don't would to lost a possibility to simple call check function.

Regards

Pavel

>
> Regards
>
> Pavel Stehule
>
>>
>> --
>> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
>> The PostgreSQL Company - Command Prompt, Inc.
>> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of sáb mar 03 02:45:06 -0300 2012:

> > Without correct registration you cannot to call PL check function
> > directly simply. I don't thing so this is good price for removing a
> > few SPI lines. I don't understand why you don't like SPI.

I don't dislike SPI in general. I just dislike using it internally in
the backend. Other than RI, it's not used anywhere.

> > It is used more times in code for similar purpose.
>
> this disallow direct PL check function call - so any more complex
> situation cannot be solved by SQL, but must be solved by PL/pgSQL with
> dynamic SQL

Nonsense. Where did you get this idea? I did not touch the plpgsql
code at all, it'd still work exactly as in your original patch.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/3/3 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Pavel Stehule's message of sáb mar 03 02:45:06 -0300 2012:
>
>> > Without correct registration you cannot to call PL check function
>> > directly simply. I don't thing so this is good price for removing a
>> > few SPI lines. I don't understand why you don't like SPI.
>
> I don't dislike SPI in general.  I just dislike using it internally in
> the backend.  Other than RI, it's not used anywhere.

>
>> > It is used more times in code for similar purpose.
>>
>> this disallow direct PL check function call - so any more complex
>> situation cannot be solved by SQL, but must be solved by PL/pgSQL with
>> dynamic SQL
>
> Nonsense.  Where did you get this idea?  I did not touch the plpgsql
> code at all, it'd still work exactly as in your original patch.

ok

I am sorry

Pavel

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 03/03/2012 02:24 AM, Alvaro Herrera wrote:
> question: how attached you are to the current return format for CHECK
> FUNCTION?
>
> check function f1();
> CHECK FUNCTION
> -------------------------------------------------------------
> In function: 'f1()'
> error:42804:5:assignment:subscripted object is not an array
> (2 rows)
>
> It seems to me that it'd be trivial to make it look like this instead:
>
> check function f1();
> function | lineno | statement | sqlstate | message | detail | hint | level | position | query
> ---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
> f1() | 5 | assignment | 42804 | subscripted object is not an array | | | error | |
> (1 row)
>
> This looks much nicer to me.
>
> One thing we lose is the caret marking the position of the error -- but
> I'm wondering if that really works well. I didn't test it but from the
> code it looks to me like it'd misbehave if you had a multiline statement.
>
> Opinions?

Well, if you want nicely formated table you can always call the checker
function directly, I think the statement returning something that is
more human and less machine is more consistent approach with the rest of
the utility commands. In other words I don't really see the point of it.

Petr

Excerpts from Petr Jelínek's message of sáb mar 03 10:26:04 -0300 2012:
> On 03/03/2012 02:24 AM, Alvaro Herrera wrote:
> > question: how attached you are to the current return format for CHECK
> > FUNCTION?
> >
> > check function f1();
> > CHECK FUNCTION
> > -------------------------------------------------------------
> > In function: 'f1()'
> > error:42804:5:assignment:subscripted object is not an array
> > (2 rows)

> Well, if you want nicely formated table you can always call the checker
> function directly, I think the statement returning something that is
> more human and less machine is more consistent approach with the rest of
> the utility commands. In other words I don't really see the point of it.

I am not against having some more human readable output than plain
tabular. In particular the idea that we need to have all fields is of
course open to discussion. But is the output as proposed above really
all that human friendly? I disagree that it cannot be improved.

BTW one thing that's missing in this feature so far is some
translatability of the returned output.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012:
> Hello
>
> >
> > It wasn't all that difficult -- see below.  While at this, I have a
> > question: how attached you are to the current return format for CHECK
> > FUNCTION?
>
> TupleDescr is created by language creator. This ensure exactly
> expected format, because there are no possible registry check function
> with other output tuple descriptor.

I'm not sure what you're saying. What TupDesc are you talking about?
The tupdesc returned by the checker is certainly hardcoded by the core
support; the language creator cannot deviate from it.

> > check function f1();
> > function | lineno | statement  | sqlstate |              message               | detail | hint | level | position | query
> > ---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
> > f1()     |      5 | assignment | 42804    | subscripted object is not an array |        |      | error |          |
> > (1 row)
> >
> > This looks much nicer to me.
>
> I am strongly disagree.
>
> 1. This format is not consistent with other commands,
> 2. This format is difficult for copy/paste
> 3. THE ARE NOT CARET - this is really important
> 5. This form is bad for terminals - there are long rows, and with \x
> outout, there are lot of "garbage" for multicommand
> 4. When you would to this form, you can to directly call SRF PL check
> functions.

I am not sure that consistency is the most important thing here; I think
what we care about is that it's usable. So yeah, it might be hard to
cut and paste, and also too wide. Maybe we can run some of the fields
together, and omit others.

I am not sure about the caret thingy -- mainly because I don't think it
works all that well. I don't know how psql does it, but I notice that
it shows a single line in a multiline query -- so it's not just a matter
of adding some number of spaces.

Given the negative feedback, I'm going to leave this output format
unchanged; we can tweak it later.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012:
> Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012:

> > 3. THE ARE NOT CARET - this is really important

> I am not sure about the caret thingy -- mainly because I don't think it
> works all that well. I don't know how psql does it, but I notice that
> it shows a single line in a multiline query -- so it's not just a matter
> of adding some number of spaces.

I checked how this works in psql. It is upwards of 200 lines of code --
see reportErrorPosition in libpq/fe-protocol3.c. I'm not sure this can
be made to work sensibly here.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of sáb mar 03 17:56:23 -0300 2012:
> Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012:
> > Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012:
>
> > > 3. THE ARE NOT CARET - this is really important
>
> > I am not sure about the caret thingy -- mainly because I don't think it
> > works all that well.

It doesn't work correctly with your patch; see sample below. Note the
caret is pointing to an entirely nonsensical position. I'm not sure
about duplicating the libpq line-counting logic in the backend.

Also note i18n seems to be working well, except for the "In function"
header, "query", and the error level. That seems easily fixable.

I remain unconvinced that this is the best possible output.

alvherre=# create function f() returns int language plpgsql as $$
begin select
var
from
foo; end; $$;
CREATE FUNCTION
alvherre=# check function f();
CHECK FUNCTION
---------------------------------------------------------
In function: 'f()'
error:42P01:2:sentencia SQL:no existe la relación «foo»
query:select +
var +
from +
foo
^
(4 filas)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/3/4 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Alvaro Herrera's message of sáb mar 03 17:56:23 -0300 2012:
>> Excerpts from Alvaro Herrera's message of sáb mar 03 16:54:19 -0300 2012:
>> > Excerpts from Pavel Stehule's message of sáb mar 03 02:25:52 -0300 2012:
>>
>> > > 3. THE ARE NOT CARET - this is really important
>>
>> > I am not sure about the caret thingy -- mainly because I don't think it
>> > works all that well.
>
> It doesn't work correctly with your patch; see sample below.  Note the
> caret is pointing to an entirely nonsensical position.  I'm not sure
> about duplicating the libpq line-counting logic in the backend.
>
> Also note i18n seems to be working well, except for the "In function"
> header, "query", and the error level.  That seems easily fixable.
>
> I remain unconvinced that this is the best possible output.
>
> alvherre=# create function f() returns int language plpgsql as $$
> begin select
> var
> from
> foo; end; $$;
> CREATE FUNCTION
> alvherre=# check function f();
>                     CHECK FUNCTION
> ---------------------------------------------------------
>  In function: 'f()'
>  error:42P01:2:sentencia SQL:no existe la relación «foo»
>  query:select                                           +
>  var                                                    +
>  from                                                   +
>  foo
>                       ^
> (4 filas)
>

this should be fixed. I checked expressions, that works (I expect)
correctly. Caret helps - (really). Sometimes man is blind :).

I'll look on this topic tomorrow and I hope this will be solvable simply.

Regards

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of dom mar 04 16:33:08 -0300 2012:
>
> Hello
>
> 2012/3/4 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:

> >                     CHECK FUNCTION
> > ---------------------------------------------------------
> >  In function: 'f()'
> >  error:42P01:2:sentencia SQL:no existe la relación «foo»
> >  query:select                                           +
> >  var                                                    +
> >  from                                                   +
> >  foo
> >                       ^
> > (4 filas)
> >
>
> this should be fixed. I checked expressions, that works (I expect)
> correctly. Caret helps - (really). Sometimes man is blind :).

Agreed.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

2012/3/5 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Pavel Stehule's message of dom mar 04 16:33:08 -0300 2012:
>>
>> Hello
>>
>> 2012/3/4 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
>> >                     CHECK FUNCTION
>> > ---------------------------------------------------------
>> >  In function: 'f()'
>> >  error:42P01:2:sentencia SQL:no existe la relación «foo»
>> >  query:select                                           +
>> >  var                                                    +
>> >  from                                                   +
>> >  foo
>> >                       ^
>> > (4 filas)
>> >
>>
>> this should be fixed. I checked expressions, that works (I expect)
>> correctly. Caret helps - (really). Sometimes man is blind :).
>
> Agreed.
>

I don't have your last version, so I am sending just part of
CheckFunctionById function - this fragment ensures a output

or please, send me your last patch and I'll do merge

now result is better

postgres=> create function f() returns int language plpgsql as $$
postgres$> begin select
postgres$> var
postgres$> from
postgres$> foo; end; $$;
CREATE FUNCTION
postgres=> check function f();
CHECK FUNCTION
-----------------------------------------------------------
In function: f()
error:42P01:2:SQL statement:relation "foo" does not exist
query:select
var
from
foo
^
(7 rows)

and some utf8 fce

postgres=> check function fx(int);
CHECK FUNCTION
--------------------------------------------------
In function: fx(integer)
error:42703:3:RETURN:column "ýšý" does not exist
query:SELECT (select žlutý
from jj
/* ýšý */
where /*ýšýšý8*/ ýšý = 10)
^
(7 rows)

postgres=> check function fx(int);
CHECK FUNCTION
-------------------------------------------------
In function: fx(integer)
error:42703:3:RETURN:column "xx" does not exist
query:SELECT (select t.a
from t
/* ýšý */
where /*ýšýšý8*/ xx = 10)
^
(7 rows)

caret is ok

regards

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

small fix of CheckFunctionById function

Regards

p.s. Alvaro, please, send your patch and I'll merge it

Excerpts from Pavel Stehule's message of lun mar 05 13:02:50 -0300 2012:
> small fix of CheckFunctionById function
>
> Regards
>
> p.s. Alvaro, please, send your patch and I'll merge it

Here it is, with your changes already merged. I also added back the
new reference doc files which were dropped after the 2012-01-01 version.
Note I haven't touched or read the plpgsql checker code at all (only
some automatic indentation changes IIRC). I haven't verified the
regression tests either.

FWIW I'm not going to participate in the other thread; neither I am
going to work any more on this patch until the other thread sees some
reasonable conclusion.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

* I refreshed regress tests and appended tests for multi lines query
* There are enhanced checking of SELECT INTO statement
* I fixed showing details and hints

Regards

Pavel Stehule

2012/3/5 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of lun mar 05 13:02:50 -0300 2012:
>> small fix of CheckFunctionById function
>>
>> Regards
>>
>> p.s. Alvaro, please, send your patch and I'll merge it
>
> Here it is, with your changes already merged.  I also added back the
> new reference doc files which were dropped after the 2012-01-01 version.
> Note I haven't touched or read the plpgsql checker code at all (only
> some automatic indentation changes IIRC).  I haven't verified the
> regression tests either.
>
> FWIW I'm not going to participate in the other thread; neither I am
> going to work any more on this patch until the other thread sees some
> reasonable conclusion.
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Pavel Stehule's message of mar mar 06 03:43:06 -0300 2012:
> Hello
>
> * I refreshed regress tests and appended tests for multi lines query
> * There are enhanced checking of SELECT INTO statement
> * I fixed showing details and hints

Oh, I forgot to remove the do_tup_output_slot() function which I added
in some previous version but is no longer necessary. Also, there are
two includes that I put separately in functioncmds.c that are only
necessary for the CHECK FUNCTION stuff; those should be merged in with
the other includes there. (I was toying with the idea of moving all
that code to a new file).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/3/6 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>
> Excerpts from Pavel Stehule's message of mar mar 06 10:44:09 -0300 2012:
>>
>> 2012/3/6 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>> >
>> > Excerpts from Pavel Stehule's message of mar mar 06 03:43:06 -0300 2012:
>> >> Hello
>> >>
>> >> * I refreshed regress tests and appended tests for multi lines query
>> >> * There are enhanced checking of SELECT INTO statement
>> >> * I fixed showing details and hints
>> >
>> > Oh, I forgot to remove the do_tup_output_slot() function which I added
>> > in some previous version but is no longer necessary.  Also, there are
>> > two includes that I put separately in functioncmds.c that are only
>> > necessary for the CHECK FUNCTION stuff; those should be merged in with
>> > the other includes there.  (I was toying with the idea of moving all
>> > that code to a new file).
>> >
>>
>> I am not sure, what you did do. Can you remove this useless code, please?
>
> It's just a three line function that's not called anywhere.

ok

fixed patch

Pavel

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

there is new version

* fixed small formatting issues related to drop SPI call
* long functions was divided
* CREATE TRIGGER ALL ON table support

Regards

Pavel

Hello

I found one small issue where Query was not forwarded when trigger
record was broken. I had to append "context" forwarding.

Regards

Pavel

2012/3/6 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> Hello
>
> there is new version
>
> * fixed small formatting issues related to drop SPI call
> * long functions was divided
> * CREATE TRIGGER ALL ON table support
>
> Regards
>
> Pavel

Hello

multicheck for triggers are supported now

CHECK TRIGGER ALL;
CHECK TRIGGER ALL IN SCHEMA xxx FOR ROLE yyy;

Regards

Pavel Stehule

What happened to this feature patch? A TODO?

---------------------------------------------------------------------------

On Tue, Nov 29, 2011 at 08:37:15PM +0100, Pavel Stehule wrote:
> 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
>
> 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
> >