Re: Improve error handling in pltcl

Hi

2016-03-13 20:24 GMT+01:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>:

> On 3/3/16 8:51 AM, Pavel Stehule wrote:
>
>> Hi
>>
>> I am testing behave, and some results looks strange
>>
>
> Thanks for the review!
>
> postgres=# \sf foo
>> CREATE OR REPLACE FUNCTION public.foo()
>> RETURNS void
>> LANGUAGE plpgsql
>> AS $function$
>> begin
>> raise exception sqlstate 'ZZ666' using message='hello, world',
>> detail='hello, my world', hint = 'dont afraid';
>> end
>> $function$
>>
>> postgres=# select tcl_eval('spi_exec "select foo();"');
>> ERROR: 38000: hello, world
>> CONTEXT: hello, world <<<<==========???????
>>
>> the message was in context. Probably it is out of scope of this patch,
>> but it isn't consistent with other PL
>>
>>
>> while executing
>> "spi_exec "select foo();""
>> ("eval" body line 1)
>> invoked from within
>> "eval $1"
>> (procedure "__PLTcl_proc_16864" line 3)
>> invoked from within
>> "__PLTcl_proc_16864 {spi_exec "select foo();"}"
>> in PL/Tcl function "tcl_eval"
>> LOCATION: throw_tcl_error, pltcl.c:1217
>> Time: 1.178 ms
>>
>
> Both problems actually exists in HEAD. The issue is this line in
> throw_tcl_error:
>
> econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
> TCL_GLOBAL_ONLY));
>
> Offhand I don't see any great way to improve that behavior, and in any
> case it seems out of scope for this patch. As a workaround I'm just forcing
> psql error VERBOSITY to terse for now.
>

I understand - it is unpleasant, but it is another scope.

>
> postgres=# select tcl_eval('join $::errorCode "\n"');
>> tcl_eval
>> ═════════════════════════════════════════
>> POSTGRES ↵
>> message ↵
>> hello, world ↵
>> detail ↵
>> hello, my world ↵
>> hint ↵
>> dont afraid ↵
>> domain ↵
>> plpgsql-9.6 ↵
>> context_domain ↵
>> postgres-9.6 ↵
>> context ↵
>> PL/pgSQL function foo() line 3 at RAISE↵
>> SQL statement "select foo();" ↵
>> cursor_position ↵
>> 0 ↵
>> filename ↵
>> pl_exec.c ↵
>> lineno ↵
>> 3165 ↵
>> funcname ↵
>> exec_stmt_raise
>> (1 row)
>>
>> I miss a SQLSTATE.
>>
>
> Great catch. Fixed.
>

I can verify it. The doc should be updated too.

>
> Why is used List object instead dictionary? TCL supports it
>> https://www.tcl.tk/man/tcl8.5/tutorial/Tcl23a.html
>>
>
> Because errorCode unfortunately is an array and not a dict. It doesn't
> really seem worth messing with it in the eval since this is just a sanity
> check...
>

I am sorry, my I expected so we introduced errorCode. My question was not
valid

> New patch attached. It also removes some other unstable output from the
> regression test.
>

I checked this patch:

* This patch is relative trivial without any controversy - allow a access
to ErrorData fields is good idea, and we do it in some other PL longer time.
* There are no problem with patching, compiling
* all tests was passed
* a comments in code are adequate to low complexity
* code respects PostgreSQL formatting
* attached documentation is good and correct
* regress tests are adequate

I fixed the documentation - there was not information about SQLSTATE field.
See, please, attachment.

I'll mark this patch as ready for commiters.

Regards

Pavel

> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>