Re: review: CHECK FUNCTION statement

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