Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

On 3/25/16 3:11 PM, Tom Lane wrote:

Jim Nasby  writes:
the data, we're making it unnecessarily hard.  All we need is one more
field in there, and you can simplify that to


Ahh, nice.


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.


Ahh, ok, finally I get it.

It would be nice if the comments for ErrorData were clearer...


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


Yeah, Karl and I were just talking about that as we were finishing up 
the docs changes (ironically, as you were commiting this...).


I ended up with a more realistic example that also demonstrates that you 
can refer to errorCode in a separate function if desired. That patch 
attached for posterity.

--
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
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index d2175d5..4cf32df 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -775,6 +775,169 @@ CREATE EVENT TRIGGER tcl_a_snitch ON ddl_command_start 
EXECUTE PROCEDURE tclsnit
 

 
+   
+Error Handling in PL/Tcl
+
+
+ error handling
+ in PL/Tcl
+
+
+
+ All Tcl errors that occur within a stored procedure and are not caught
+ using Tcl's catch or try
+ functions will raise a database error.
+
+
+ Tcl code can raise a database error by invoking the
+ elog command provided by PL/Tcl or by generating an
+ error using the Tcl error command and not catching it
+ with Tcl's catch command.
+
+
+ Database errors that occur from the PL/Tcl stored procedure's
+ use of spi_exec, spi_prepare,
+ and spi_execp are also catchable by Tcl's
+ catch command.
+
+
+ Tcl provides an errorCode variable that can represent
+ additional information about the error in a form that is easy for programs
+ to interpret.  The contents are a Tcl list format. The first word
+ identifies the subsystem or library responsible for the error. The
+ remaining contents are up to the individual code or library.  For example
+ if Tcl's open command is asked to open a file that
+ doesn't exist, errorCode might contain POSIX
+ ENOENT {no such file or directory} where the third element may
+ vary by locale but the first and second will not.
+
+
+ When spi_exec, spi_prepare
+ or spi_execp cause a database error to be raised,
+ that database eror propagates back to Tcl as a Tcl error.  In this case
+ errorCode is set to a list where the first element is
+ POSTGRES followed by details of the Postgres error.
+ Since fields in the structure may or may not be present depending on the
+ nature of the error, how the function was invoked, etc, PL/Tcl has adopted
+ the convention that subsequent elements of the
+ errorCode list are key-value pairs where the first
+ value is the name of the field and the second is its value.
+
+
+ Fields that may be present include SQLSTATE,
+ message,
+ detail,
+ hint,
+ context,
+ schema,
+ table,
+ column,
+ datatype,
+ constraint,
+ cursor_position,
+ internalquery,
+ internal_position,
+ filename,
+ lineno and
+ funcname.
+
+
+ You might find it useful to load the results into an array. Code
+ for doing that might look like
+
+if {[lindex $errorCode 0] == "POSTGRES"} {
+array set errorRow [lrange $errorCode 1 end]
+}
+
+
+
+ This example shows how to trap a specific SQL error.
+
+CREATE TABLE account(user_name varchar(1) NOT NULL PRIMARY KEY);
+CREATE OR REPLACE FUNCTION public.create_user(user_name text)
+ RETURNS void LANGUAGE pltcl AS $function$
+set prep [ spi_prepare "INSERT INTO account(user_name) VALUES(\$1)" [ list 
text ] ]
+if [ catch {
+spi_execp $prep [ list $1 ]
+} msg ] {
+if {[lindex $::errorCode 0] == "POSTGRES"} {
+array set errorData [lrange $::errorCode 1 end]
+if { $errorData(SQLSTATE) == "23505" && $errorData(constraint) == 
"account_pkey" } {
+return -code error "user '$1' already exists"
+}
+}
+throw $::errorCode $msg
+}
+$function$;
+
+
+
+SELECT create_user('a');
+ create_user 
+-
+ 
+(1 row)
+
+SELECT create_user('a');
+ERROR: 

Re: [HACKERS] Improve error handling in pltcl

[Tom Lane]

Jim Nasby  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

On 3/20/16 8:42 PM, Jim Nasby wrote:

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 looked at the
plpython stuff and I'm still really unclear on what exactly an
internalquery is as opposed to regular context info?
PLy_spi_exception_set simply exposes the raw internalquery and internalpos.


Anyone any pointers on this? I'm not sure I can finish the docs without 
knowing what we want to do here.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

On 3/17/16 5:46 PM, Tom Lane wrote:

Pavel Stehule  writes:

I'll mark this patch as ready for commiters.


I started to look at this patch.  It seems to me that the format of the
errorCode output is not terribly well designed.

...

Maybe there's another way.  I've not used Tcl in anger since around
the turn of the century, so it's entirely likely that I'm missing
something.  But the proposed doc addition isn't showing me any really
easy way to work with this data format, and I think that that's either
a design fail or a docs fail, not something I should just accept as
the best we can do.


I asked Karl about this (since he's active in the TCL community and 
works with TCL every day), and his response was essentially:


Tcl is all about flat lists of key value pairs.

array set myArray $list

sucks a flat list of key-value pairs into an array and vice versa

set list [array get myArray]

creates one. This is normal Tcl stuff.

Getting the errorCode into an array is as easy as

array set errorData [lrange $errorCode 1 end]

Then you can do

$errorData(detail), $errorData(message), etc.

In fact keyed lists in TclX which are the inspiration for the approach 
to lists of alternating key-value pairs did it the way he suggested and 
that’s fallen by the wayside in favor of flat lists.


There has been a formal proposal to add a -stride to lsearch to make 
lsearch efficient at searching the same flat lists of key-value pairs 
and I expect to see it in Tcl 8.7 or sooner.



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 looked at the 
plpython stuff and I'm still really unclear on what exactly an 
internalquery is as opposed to regular context info? 
PLy_spi_exception_set simply exposes the raw internalquery and internalpos.



Also, I'm curious as to why you think "domain" or "context_domain"
is of any value to expose here.  Tcl code is not going to have any
access to the NLS infrastructure (if that's even been compiled) to
do anything with those values.


I'm not really sure what it's hurting to expose that, but I'll remove it.


And I believe it may be a security violation for this code to expose
"detail_log".  The entire point of that field is it goes to the
postmaster log and NOT anywhere where unprivileged clients can see it.


Removed.


Nitpickier stuff:

* Docs example could use work: it should show how to do something
useful *in Tcl code*, like maybe checking whether an error had a
particular SQLSTATE.  The example with dumping the whole list at the
psql command line is basically useless, not to mention that it
relies on a nonexistent "tcl_eval" function.  (And I don't care


Will work on an improved example.


for the regression test case creating such a function ... isn't
that a fine SQL-injection hole?)


If it was taking external input, but it's not, and it saves from 
creating 2 separate functions (which you want to do to make sure the 
global errorCode is being set, and not a local copy).



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


* Useless-looking hunk at pltcl.c:1610


Removed.


* I think the unstable data you're griping about is the Tcl function's
OID, not the PID.  (I wonder whether we should make an effort to hide
that in errorInfo.  But if so it's a matter for a separate patch.)


It's possible that someone would want to know the name of the 
constructed TCL function (and yeah, I think it's the OID not PID).



I'll set this patch back to Waiting On Author.  I believe it's well
within reach of getting committed in this fest, but it needs more
work.


Interim patch attached (need to work on the docs).
--
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
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index d2175d5..d5c576d 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -775,6 +775,127 @@ CREATE EVENT TRIGGER tcl_a_snitch ON ddl_command_start 
EXECUTE PROCEDURE tclsnit
 

 
+   
+Error Handling in PL/Tcl
+
+
+ error handling
+ in PL/Tcl
+
+
+
+ All Tcl errors that are allowed to propagate back to the top level of the
+ interpreter, that is, errors not caught within the stored procedure
+ using the Tcl catch command will raise a database
+ error.
+
+
+ Tcl code within or called from the stored procedure can choose to
+ raise a database error by invoking the elog
+ command provided by PL/Tcl or by generating an error using the Tcl
+ error command and not 

Re: [HACKERS] Improve error handling in pltcl

[Tom Lane]

Pavel Stehule  writes:
> I'll mark this patch as ready for commiters.

I started to look at this patch.  It seems to me that the format of the
errorCode output is not terribly well designed.  I see that Tcl constrains
it to be a list starting with an error-class-identifying keyword, for
which you've chosen POSTGRES.  So far fine.  But for the rest of the list,
you've chosen a format of alternating keywords and values, wherein some of
the pairs may be missing, and (I presume) we reserve the right to invent
new keywords.  Admittedly my Tcl is pretty rusty, but this seems to me to
be not easy to deal with.  The errorCode list format is really designed on
the assumption that any particular error-class-identifying keyword implies
a fixed format for the rest of the list, so you can pull out the fields
you care about with lindex.  But here, users cannot safely assume that any
particular value is at a specific list index, which means they've got to
search for the keyword they want, and this representation doesn't make
that very easy AFAICS.  The doc text proposes loading all but the POSTGRES
keyword into a Tcl array, which solves the problem since the keywords
become knowable array subscripts, but even that is pretty awkward because
you've got to leave out POSTGRES.  (Also, using the array is a bit awkward
if you aren't sure whether an entry is present, no?)

So I think this needs to be redesigned to make it less painful to pull
out the value for a given keyword.  I'm not very clear on what's the
best way, though.

One line of thought is to make the format use sublists:

POSTGRES {keyword value} {keyword value} ...

This could be searched with lsearch, eg to get the SQLSTATE

lindex [lsearch -index 0 -inline $errorCode SQLSTATE] 1

but that still seems pretty awkward (maybe there's a better way?)

Another idea is to put some junk value after POSTGRES (maybe the server
version number?) with the idea that then it would be trivial to load
the data into an array with "array set".  But you'd really want to
document it as that's what you MUST do to extract data, not that it's
an optional solution.

Maybe there's another way.  I've not used Tcl in anger since around
the turn of the century, so it's entirely likely that I'm missing
something.  But the proposed doc addition isn't showing me any really
easy way to work with this data format, and I think that that's either
a design fail or a docs fail, not something I should just accept as
the best we can do.

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.

Also, I'm curious as to why you think "domain" or "context_domain"
is of any value to expose here.  Tcl code is not going to have any
access to the NLS infrastructure (if that's even been compiled) to
do anything with those values.

And I believe it may be a security violation for this code to expose
"detail_log".  The entire point of that field is it goes to the
postmaster log and NOT anywhere where unprivileged clients can see it.

Nitpickier stuff:

* Docs example could use work: it should show how to do something
useful *in Tcl code*, like maybe checking whether an error had a
particular SQLSTATE.  The example with dumping the whole list at the
psql command line is basically useless, not to mention that it
relies on a nonexistent "tcl_eval" function.  (And I don't care
for the regression test case creating such a function ... isn't
that a fine SQL-injection hole?)

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

* Useless-looking hunk at pltcl.c:1610

* I think the unstable data you're griping about is the Tcl function's
OID, not the PID.  (I wonder whether we should make an effort to hide
that in errorInfo.  But if so it's a matter for a separate patch.)

I'll set this patch back to Waiting On Author.  I believe it's well
within reach of getting committed in this fest, but it needs more
work.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Pavel Stehule]

Hi

2016-03-13 20:24 GMT+01:00 Jim Nasby :

> 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
>
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
new file mode 100644
index d2175d5..1ac7804
*** a/doc/src/sgml/pltcl.sgml
--- b/doc/src/sgml/pltcl.sgml
*** CREATE EVENT TRIGGER tcl_a_snitch ON ddl
*** 775,780 
--- 775,903 
  
 
  
+
+ Error Handling in PL/Tcl
+ 
+ 
+  error handling
+  in PL/Tcl
+ 
+ 
+ 
+  All Tcl errors that are allowed to propagate back to the top level of the
+  interpreter, that is, errors not caught within the stored procedure
+  using the Tcl catch command will raise a database
+  error.
+ 
+ 
+  Tcl code within or called from the stored procedure can choose to
+  raise a database error by invoking the elog
+  command provided by PL/Tcl or by generating an error using the Tcl
+  error command and not catching it with Tcl's
+  catch command.
+ 
+ 
+  Database errors that occur from the PL/Tcl stored procedure's
+  use of spi_exec, spi_prepare,

Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

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.



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.


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...


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

--
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
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
index d2175d5..d5c576d 100644
--- a/doc/src/sgml/pltcl.sgml
+++ b/doc/src/sgml/pltcl.sgml
@@ -775,6 +775,127 @@ CREATE EVENT TRIGGER tcl_a_snitch ON ddl_command_start 
EXECUTE PROCEDURE tclsnit
 

 
+   
+Error Handling in PL/Tcl
+
+
+ error handling
+ in PL/Tcl
+
+
+
+ All Tcl errors that are allowed to propagate back to the top level of the
+ interpreter, that is, errors not caught within the stored procedure
+ using the Tcl catch command will raise a database
+ error.
+
+
+ Tcl code within or called from the stored procedure can choose to
+ raise a database error by invoking the elog
+ command provided by PL/Tcl or by generating an error using the Tcl
+ error command and not catching it with Tcl's
+ catch command.
+
+
+ Database errors that occur from the PL/Tcl stored procedure's
+ use of spi_exec, spi_prepare,
+ and spi_execp are also catchable by Tcl's
+ catch command.
+
+
+ Tcl provides an errorCode variable that can
+ represent additional information about the error in a form that
+ is easy for programs to interpret.  The contents are in Tcl list
+ format and the first word identifies the subsystem or
+ library responsible for the error and beyond that the contents are left
+ to the individual code or library.  For example if Tcl's
+ open command is asked to open a file that doesn't
+ exist, errorCode
+ might contain POSIX ENOENT {no such file or directory}
+ where the third element may vary by locale but the first and second
+ will not.
+
+
+ When spi_exec, spi_prepare
+ or spi_execp cause a database error to be raised,
+ that database eror propagates back to Tcl as a Tcl error.
+ In this case errorCode is set to a list
+ where the first element is POSTGRES followed by a
+ copious decoding of the Postgres error structure.  Since fields in the
+ structure may or may not be present depending on 

Re: [HACKERS] Improve error handling in pltcl

[Robert Haas]

On Thu, Mar 3, 2016 at 9:51 AM, Pavel Stehule  wrote:
> I am testing behave, and some results looks strange

Jim, this is waiting for you to respond to Pavel's review.  If that
doesn't happen soon, this should be marked Returned with Feedback and
you can, if you wish, resubmit for 9.7.

Thanks,

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Pavel Stehule]

Hi

I am testing behave, and some results looks strange

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

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.

Why is used List object instead dictionary? TCL supports it
https://www.tcl.tk/man/tcl8.5/tutorial/Tcl23a.html

Regards

Pavel

Re: [HACKERS] Improve error handling in pltcl

[Pavel Stehule]

Hi

2016-03-01 22:23 GMT+01:00 Jim Nasby :

> On 2/29/16 10:01 PM, Tom Lane wrote:
>
>> Jim Nasby  writes:
>>
>>> On 2/28/16 5:50 PM, Jim Nasby wrote:
>>>
 Per discussion in [1], this patch improves error reporting in pltcl.

>>>
>> I forgot to mention that this work is sponsored by Flight Aware
>>> (http://flightaware.com).
>>>
>>
>> Huh ... I use that site.  There's PG and pltcl code behind it?
>> Cool!
>>
>
>
I didn't study this patch deeper yet. Just I am sending rebased code

I found one issue. The context

+ ERROR:  relation "foo" does not exist
+ CONTEXT:  relation "foo" does not exist
+ while executing
+ "spi_exec "select * from foo;""
+ ("eval" body line 1)
+ invoked from within
+ "eval $1"
+ (procedure "__PLTcl_proc_16461" line 3)
+ invoked from within
+ "__PLTcl_proc_16461 {spi_exec "select * from foo;"}"
+ in PL/Tcl function "tcl_eval"

is changed in any call - when I did "make installcheck"


*** 567,575 
  ("eval" body line 1)
  invoked from within
  "eval $1"
! (procedure "__PLTcl_proc_16461" line 3)
  invoked from within
! "__PLTcl_proc_16461 {spi_exec "select * from foo;"}"
  in PL/Tcl function "tcl_eval"
  select pg_temp.tcl_eval($$
  set list [lindex $::errorCode 0];
--- 567,575 
  ("eval" body line 1)
  invoked from within
  "eval $1"
! (procedure "__PLTcl_proc_16841" line 3) <
_PLTcl_proc_
  invoked from within
! "__PLTcl_proc_16841 {spi_exec "select * from foo;"}"
  in PL/Tcl function "tcl_eval"
  select pg_temp.tcl_eval($$
  set list [lindex $::errorCode 0];

I am not able to see, if this information is interesting or not. We can
hide context, but I have not a idea, if it is ok or not.

Regards

Pavel
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
new file mode 100644
index d2175d5..d5c576d
*** a/doc/src/sgml/pltcl.sgml
--- b/doc/src/sgml/pltcl.sgml
*** CREATE EVENT TRIGGER tcl_a_snitch ON ddl
*** 775,780 
--- 775,901 
  
 
  
+
+ Error Handling in PL/Tcl
+ 
+ 
+  error handling
+  in PL/Tcl
+ 
+ 
+ 
+  All Tcl errors that are allowed to propagate back to the top level of the
+  interpreter, that is, errors not caught within the stored procedure
+  using the Tcl catch command will raise a database
+  error.
+ 
+ 
+  Tcl code within or called from the stored procedure can choose to
+  raise a database error by invoking the elog
+  command provided by PL/Tcl or by generating an error using the Tcl
+  error command and not catching it with Tcl's
+  catch command.
+ 
+ 
+  Database errors that occur from the PL/Tcl stored procedure's
+  use of spi_exec, spi_prepare,
+  and spi_execp are also catchable by Tcl's
+  catch command.
+ 
+ 
+  Tcl provides an errorCode variable that can
+  represent additional information about the error in a form that
+  is easy for programs to interpret.  The contents are in Tcl list
+  format and the first word identifies the subsystem or
+  library responsible for the error and beyond that the contents are left
+  to the individual code or library.  For example if Tcl's
+  open command is asked to open a file that doesn't
+  exist, errorCode
+  might contain POSIX ENOENT {no such file or directory}
+  where the third element may vary by locale but the first and second
+  will not.
+ 
+ 
+  When spi_exec, spi_prepare
+  or spi_execp cause a database error to be raised,
+  that database eror propagates back to Tcl as a Tcl error.
+  In this case errorCode is set to a list
+  where the first element is POSTGRES followed by a
+  copious decoding of the Postgres error structure.  Since fields in the
+  structure may or may not be present depending on the nature of the
+  error, how the function was invoked, etc, PL/Tcl has adopted the 
+  convention that subsequent elements of the errorCode
+  list are key-value pairs where the first value is the name of the
+  field and the second is its value.
+ 
+ 
+  Fields that may be present include message,
+  detail, detail_log,
+  hint, domain,
+  context_domain, context,
+  schema, table,
+  column, datatype,
+  constraint, cursor_position,
+  internalquery, internal_position,
+  filename, lineno and
+  funcname.
+ 
+ 
+  You might find it useful to load the results into an array. Code
+  for doing that might look like
+ 
+ if {[lindex $errorCode 0] == "POSTGRES"} {
+ array set errorRow [lrange $errorCode 1 end]
+ }
+ 
+ 
+ 
+  In the example below we cause an error by attempting to
+  SELECT from a table that doesn't exist.
+ 
+ select tcl_eval('spi_exec "select * from foo;"');
+ 
+ 
+ 
+ ERROR:  relation "foo" does not exist
+ 
+ 
+ 

Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

On 2/29/16 10:01 PM, Tom Lane wrote:

Jim Nasby  writes:

On 2/28/16 5:50 PM, Jim Nasby wrote:

Per discussion in [1], this patch improves error reporting in pltcl.



I forgot to mention that this work is sponsored by Flight Aware
(http://flightaware.com).


Huh ... I use that site.  There's PG and pltcl code behind it?
Cool!


Heh, I didn't realize you were a TCL fan.

They've been heavy PG users from the start. Eventually PG had trouble 
keeping up with the in-flight tracking so they created Speed Tables [1]. 
And Karl (one of the founders) is a well known TCL contributor[2].


When it comes to sheer geek factor though, I think the 
multilateration[3] stuff they're doing with their ADS-B network is a 
really cool data application. It's basically a form of "reverse GPS" for 
tracking aircraft.


[1] https://github.com/flightaware/speedtables
[2] http://wiki.tcl.tk/83
[3] http://flightaware.com/adsb/mlat/
--
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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Tom Lane]

Jim Nasby  writes:
> On 2/28/16 5:50 PM, Jim Nasby wrote:
>> Per discussion in [1], this patch improves error reporting in pltcl.

> I forgot to mention that this work is sponsored by Flight Aware 
> (http://flightaware.com).

Huh ... I use that site.  There's PG and pltcl code behind it?
Cool!

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve error handling in pltcl

[Jim Nasby]

On 2/28/16 5:50 PM, Jim Nasby wrote:

Per discussion in [1], this patch improves error reporting in pltcl.


I forgot to mention that this work is sponsored by Flight Aware 
(http://flightaware.com).

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers