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