Re: review: CHECK FUNCTION statement

2012/3/3 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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 <alvherre(at)commandprompt(dot)com>
>> The PostgreSQL Company - Command Prompt, Inc.
>> PostgreSQL Replication, Consulting, Custom Development, 24x7 support