From: | Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Improve error handling in pltcl |
Date: | 2016-03-21 01:42:05 |
Message-ID: | 56EF516D.4090605@BlueTreble.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 3/17/16 5:46 PM, Tom Lane wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
pltcl_error_master-3.patch | text/plain | 12.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Haribabu Kommi | 2016-03-21 01:44:58 | Re: Combining Aggregates |
Previous Message | Haribabu Kommi | 2016-03-21 00:59:56 | Re: pg_hba_lookup function to get all matching pg_hba.conf entries |