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@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 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@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support