Re: Improve error handling in pltcl

Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> writes:
> On 3/17/16 5:46 PM, Tom Lane wrote:
>> I started to look at this patch. It seems to me that the format of the
>> errorCode output is not terribly well designed.

> Getting the errorCode into an array is as easy as
> array set errorData [lrange $errorCode 1 end]

Well, my point is that if we expect that this is *the* way to work with
the data, we're making it unnecessarily hard. All we need is one more
field in there, and you can simplify that to

array set errorData $errorCode

which is less typing, less opportunity for mistyping, and fewer machine
cycles. I figure we can stick the Postgres version in after POSTGRES
and nobody will think that particularly odd, but it enables simpler
loading of the results into an array.

>> The doc example also makes me think that more effort should get expended
>> on converting internalquery/internalpos to just be query/cursorpos.
>> It seems unlikely to me that a Tcl function could ever see a case
>> where the latter fields are useful directly.

> Is there docs or an example on how to handle that?

I think actually it's a simple point: there won't ever be a case where
cursorpos is set here, because that's only used for top-level SQL syntax
errors. Anything we are catching would be an internal-query error, so
we might as well not confuse PL/Tcl users with the distinction but just
report internalquery/internalpos as the statement and cursor position.

> PLy_spi_exception_set simply exposes the raw internalquery and internalpos.

Right, because that's all that could be useful.

>> * I believe pltcl_construct_errorCode needs to do E2U encoding
>> conversion for anything that could contain non-ASCII data, which is
>> most of the non-fixed strings.

> Done.

Nah, you need a separate UTF_BEGIN/END pair for each one, else you're
leaking all but the last translated string. I'm not entirely sure that
it's worth the trouble to avoid such transient leaks, but as long as
PL/Tcl has got this infrastructure we should use it.

Anyway, I cleaned all that up and committed it, but as I'm sitting here
looking at the docs example I used:

if {$errorArray(SQLSTATE) == "42P01"} { # UNDEFINED_TABLE

it strikes me that this is not coding style we want to encourage.
We should borrow the infrastructure plpgsql has for converting
SQLSTATEs into condition names, so that that can be more like

if {$errorArray(condition) == "undefined_table"} {

Think I'll go fix that before I leave this subject.

regards, tom lane