Re: review: CHECK FUNCTION statement

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.

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?

-1

>
>
> /*
>  * 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

Regards

Pavel Stehule

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