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>, "PostgreSQL Hackers" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: review: CHECK FUNCTION statement
Date: 2011-11-29 12:17:01
Message-ID: D960CB61B694CF459DCFB4B0128514C2072DF447@exadv11.host.magwien.gv.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2011-11-29 13:08:45 Re: strange nbtree corruption report
Previous Message Albe Laurenz 2011-11-29 09:25:53 Re: pgsql_fdw, FDW for PostgreSQL server