Pavel Stehule <pavel.steh...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
