Re: review: CHECK FUNCTION statement - Mailing list pgsql-hackers

Excerpts from Pavel Stehule's message of mar feb 28 16:30:58 -0300 2012:
> Hello
>
> Dne 28. února 2012 17:48 Alvaro Herrera <alvherre@commandprompt.com> napsal(a):
> >
> >
> > I have a few comments about this patch:
> >
> > I didn't like the fact that the checker calling infrastructure uses
> > SPI instead of just a FunctionCallN to call the checker function.  I
> > think this should be easily avoidable.
>
> It is not possible - or it has not simple solution (I don't how to do
> it). PLpgSQL_checker is SRF function. SPI is used for processing
> returned resultset. I looked to pg source code, and I didn't find any
> other pattern than using SPI for SRF function call. It is probably
> possible, but it means some code duplication too. I invite any ideas.

It wasn't all that difficult -- see below.  While at this, I have a
question: how attached you are to the current return format for CHECK
FUNCTION?

check function f1();                      CHECK FUNCTION
-------------------------------------------------------------In function: 'f1()'error:42804:5:assignment:subscripted
objectis not an array 
(2 rows)

It seems to me that it'd be trivial to make it look like this instead:

check function f1();
function | lineno | statement  | sqlstate |              message               | detail | hint | level | position |
query 

---------+--------+------------+----------+------------------------------------+--------+------+-------+----------+-------
f1()     |      5 | assignment | 42804    | subscripted object is not an array |        |      | error |          |
(1 row)

This looks much nicer to me.

One thing we lose is the caret marking the position of the error -- but
I'm wondering if that really works well.  I didn't test it but from the
code it looks to me like it'd misbehave if you had a multiline statement.

Opinions?


/** Search and execute the checker function.**   returns true, when checked function is valid*/
static bool
CheckFunctionById(Oid funcOid, Oid relid, ArrayType *options,              bool fatal_errors, TupOutputState *tstate)
{HeapTuple        tup;Form_pg_proc    proc;HeapTuple        languageTuple;Form_pg_language lanForm;Oid
languageChecker;char          *funcname;int                result; 
tup = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcOid));if (!HeapTupleIsValid(tup)) /* should not happen */
elog(ERROR,"cache lookup failed for function %u", funcOid); 
proc = (Form_pg_proc) GETSTRUCT(tup);
languageTuple = SearchSysCache1(LANGOID, ObjectIdGetDatum(proc->prolang));Assert(HeapTupleIsValid(languageTuple));
lanForm = (Form_pg_language) GETSTRUCT(languageTuple);languageChecker = lanForm->lanchecker;
funcname = format_procedure(funcOid);
/* We're all set to call the checker */if (OidIsValid(languageChecker)){    TupleDesc        tupdesc;    Datum
 checkret;    FmgrInfo        flinfo;    ReturnSetInfo    rsinfo;    FunctionCallInfoData fcinfo; 
    /* create the tuple descriptor that the checker is supposed to return */    tupdesc = CreateTemplateTupleDesc(10,
false);   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "functionid", REGPROCOID, -1, 0);    TupleDescInitEntry(tupdesc,
(AttrNumber)2, "lineno", INT4OID, -1, 0);    TupleDescInitEntry(tupdesc, (AttrNumber) 3, "statement", TEXTOID, -1, 0);
 TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sqlstate", TEXTOID, -1, 0);    TupleDescInitEntry(tupdesc, (AttrNumber)
5,"message", TEXTOID, -1, 0);    TupleDescInitEntry(tupdesc, (AttrNumber) 6, "detail", TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc,(AttrNumber) 7, "hint", TEXTOID, -1, 0);    TupleDescInitEntry(tupdesc, (AttrNumber) 8,
"level",TEXTOID, -1, 0);    TupleDescInitEntry(tupdesc, (AttrNumber) 9, "position", INT4OID, -1, 0);
TupleDescInitEntry(tupdesc,(AttrNumber) 10, "query", TEXTOID, -1, 0);        fmgr_info(languageChecker, &flinfo); 
    rsinfo.type = T_ReturnSetInfo;    rsinfo.econtext = CreateStandaloneExprContext();    rsinfo.expectedDesc =
tupdesc;   rsinfo.allowedModes = (int) SFRM_Materialize;    /* returnMode is set by the checker, hopefully ... */    /*
isDoneis not relevant, since not using ValuePerCall */    rsinfo.setResult = NULL;    rsinfo.setDesc = NULL; 
    InitFunctionCallInfoData(fcinfo, &flinfo, 4, InvalidOid, NULL, (Node *) &rsinfo);    fcinfo.arg[0] =
ObjectIdGetDatum(funcOid);   fcinfo.arg[1] = ObjectIdGetDatum(relid);    fcinfo.arg[2] = PointerGetDatum(options);
fcinfo.arg[3]= BoolGetDatum(fatal_errors);    fcinfo.argnull[0] = false;    fcinfo.argnull[1] = false;
fcinfo.argnull[2]= false;    fcinfo.argnull[3] = false; 
    checkret = FunctionCallInvoke(&fcinfo);
    if (rsinfo.returnMode != SFRM_Materialize)        elog(ERROR, "checker function didn't return a proper return
set");   /* XXX we have to do some checking on rsinfo.isDone and checkret here */ 
    if (rsinfo.setResult != NULL)    {        bool    isnull;        StringInfoData    str;        TupleTableSlot
*slot= MakeSingleTupleTableSlot(tupdesc); 
        initStringInfo(&str);
        while (tuplestore_gettupleslot(rsinfo.setResult, true, false, slot))        {            text *message = (text
*)DatumGetPointer(slot_getattr(slot, 5, &isnull)); 
            resetStringInfo(&str);
            appendStringInfo(&str, "got a message: %s", text_to_cstring(message));
do_text_output_oneline(tstate,str.data);        } 
        pfree(str.data);        ExecDropSingleTupleTableSlot(slot);    }}
pfree(funcname);
ReleaseSysCache(languageTuple);ReleaseSysCache(tup);
return result;
}


--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support