Обсуждение: Improve error handling in pltcl
Per discussion in [1], this patch improves error reporting in pltcl. pltcl_error_objects.patch applies on top of the pltcl_objects_2.patch referenced in [2]. pltcl_error_master.patch applies against current master. [1] http://www.postgresql.org/message-id/20160223150401.2173d6b6@wagner.wagner.home [2] http://www.postgresql.org/message-id/56CCE7D2.9090005@BlueTreble.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
Вложения
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
Jim Nasby <Jim.Nasby@BlueTreble.com> 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
On 2/29/16 10:01 PM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@BlueTreble.com> 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
Hi
2016-03-01 22:23 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 2/29/16 10:01 PM, Tom Lane wrote:Jim Nasby <Jim.Nasby@BlueTreble.com> 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"
+ 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_XXXX
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];
("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_XXXX
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
Вложения
Hi
I am testing behave, and some results looks strangepostgres=# \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)
On Thu, Mar 3, 2016 at 9:51 AM, Pavel Stehule <pavel.stehule@gmail.com> 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
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
Вложения
Hi
2016-03-13 20:24 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.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
Вложения
Pavel Stehule <pavel.stehule@gmail.com> 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
On 3/17/16 5:46 PM, Tom Lane wrote: > Pavel Stehule <pavel.stehule@gmail.com> 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
Вложения
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
Jim Nasby <Jim.Nasby@BlueTreble.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
On 3/25/16 3:11 PM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@BlueTreble.com> 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