Re: review: CHECK FUNCTION statement

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