Skip site navigation (1) Skip section navigation (2)

Re: review: CHECK FUNCTION statement

From: "Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at>
To: "Pavel Stehule *EXTERN*" <pavel(dot)stehule(at)gmail(dot)com>
Cc: "PostgreSQL Hackers" <pgsql-hackers(at)postgresql(dot)org>,"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: review: CHECK FUNCTION statement
Date: 2011-11-30 15:23:16
Message-ID: D960CB61B694CF459DCFB4B0128514C2072DF93C@exadv11.host.magwien.gv.at (view raw or flat)
Thread:
Lists: pgsql-hackers
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

In response to

Responses

pgsql-hackers by date

Next:From: Pavel StehuleDate: 2011-11-30 15:42:32
Subject: Re: %TYPE and array declaration patch review
Previous:From: Tom LaneDate: 2011-11-30 14:55:18
Subject: Re: Reserved words and delimited identifiers

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group