Re: review: CHECK FUNCTION statement

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(at)commandprompt(dot)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 object is 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 ... */
/* isDone is 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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support