2012/3/3 Pavel Stehule <pavel.steh...@gmail.com>: > Hello > >> >> 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? > > TupleDescr is created by language creator. This ensure exactly > expected format, because there are no possible registry check function > with other output tuple descriptor. > >> >> 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.
This was similar to first design - it is near to result of direct PL check function call. But Tom and Albe had different opinion - check a thread three months ago, and I had to agree with them - they proposed better behave for using in psql console - and result is more similar to usual output when exception is raised. > > I am strongly disagree. > > 1. This format is not consistent with other commands, > 2. This format is difficult for copy/paste > 3. THE ARE NOT CARET - this is really important > 5. This form is bad for terminals - there are long rows, and with \x > outout, there are lot of "garbage" for multicommand > 4. When you would to this form, you can to directly call SRF PL check > functions. > >> >> 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; >> } >> > > Without correct registration you cannot to call PL check function > directly simply. I don't thing so this is good price for removing a > few SPI lines. I don't understand why you don't like SPI. It is used > more times in code for similar purpose. > > so from me -1 this disallow direct PL check function call - so any more complex situation cannot be solved by SQL, but must be solved by PL/pgSQL with dynamic SQL so I don't like it. We can talk about format for check_options - but I don't would to lost a possibility to simple call check function. Regards Pavel > > Regards > > Pavel Stehule > >> >> -- >> Álvaro Herrera <alvhe...@commandprompt.com> >> The PostgreSQL Company - Command Prompt, Inc. >> PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers