Re: pl/python tracebacks

On 26/02/11 16:10, Peter Eisentraut wrote:
> On lör, 2011-02-26 at 09:34 +0100, Jan Urbański wrote:
>> ----- Original message -----
>>> On Thu, Feb 24, 2011 at 9:03 AM, Jan Urbański <wulczer(at)wulczer(dot)org>
>>> wrote:
>>>> On 24/02/11 14:10, Peter Eisentraut wrote:
>>>> Hm, perhaps, I put it in the details, because it sounded like the place
>>>> to put information that is not that important, but still helpful. It's
>>>> kind of natural to think of the traceback as the detail of the error
>>>> message. But if you prefer context, I'm fine with that. You want me to
>>>> update the patch to put the traceback in the context?
>>>
>>> I don't see a response to this question from Peter, but I read his
>>> email to indicate that he was hoping you'd rework along these lines.
>>
>> I can do that, but not until Monday evening.
>
> Well, I was hoping for some other opinion, but I guess my request
> stands.

I looked into putting the tracebacks in the context field, but IMHO it
doesn't really play out nice. PL/Python uses a errcontext callback to
populate the context field, so the reduntant information (the name of
the function) is always there. If that callback would be removed, the
context information will not appear in plpy.warning output, which I
think would be bad.

So: the context is currently put unconditionally into every elog
message, which I think is good. In case of errors, the traceback already
includes the procedure name (because of how Python tracebacks are
typically formatted), which makes the traceback contain redundant
information to the context field. Replacing the context field with the
traceback is difficult, because it is populated by the error context
callback.

After thinking about it more I believe that the context field should
keep on being a one line indication of which function the message comes
from (and that's how it's done in PL/pgSQL for instance), and the detail
field should be used for the details of the message, like the traceback
that comes with it, and that's what the attached patch does.

While testing I noticed that this broke "raise plpy.Fatal()" behaviour -
it was no longer terminating the backend but just raising an error.
That's fixed in this version. This patch also fixes a place where
ereport is being used to report Python errors, which leads to losing the
original error. Incidentally, this is exactly the issue that made
diagnosing this bug:

http://postgresql.1045698.n5.nabble.com/Bug-in-plpython-s-Python-Generators-td3230402.html

so difficult.

Cheers,
Jan