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>
Subject: Re: review: CHECK FUNCTION statement
Date: 2011-12-16 13:49:43
Message-ID: D960CB61B694CF459DCFB4B0128514C20742FA96@exadv11.host.magwien.gv.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

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.

Yours,
Laurenz Albe

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2011-12-16 13:52:45 Re: JSON for PG 9.2
Previous Message Robert Haas 2011-12-16 13:42:56 Re: Patch to allow users to kill their own queries