Here's a patch implementing traceback support for PL/Python mentioned in
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
an incremental patch on top of the plpython-refactor patch sent eariler.

Git branch for this patch:
https://github.com/wulczer/postgres/tree/tracebacks.

It's a variant of
http://archives.postgresql.org/pgsql-patches/2006-02/msg00288.php with a
few more twists.

For errors originating from Python exceptions add the traceback as the
message detail. The patch tries to mimick Python's traceback.py module
behaviour as close as possible, icluding interleaving stack frames with
source code lines in the detail message. Any Python developer should
instantly recognize these kind of error reporting, it looks almost the
same as an error in the interactive Python shell.

A future optimisation might be not splitting the procedure source each
time a traceback is generated, but for now it's probably not the most
important scenario to optimise for.

Cheers,
Jan

Search Discussions

  • Jan Urbański at Jan 27, 2011 at 9:58 pm

    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
  • Jan Urbański at Feb 6, 2011 at 7:14 pm

    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
  • Jan Urbański at Feb 9, 2011 at 9:09 am

    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
  • Robert Haas at Feb 11, 2011 at 3:45 pm

    On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański wrote:
    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
    Alex Hunsaker is listed as the reviewer for this patch, but I don't
    see a review posted. If this feature is still wanted for 9.1, can
    someone jump in here?

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Alex Hunsaker at Feb 11, 2011 at 4:23 pm

    On Fri, Feb 11, 2011 at 08:45, Robert Haas wrote:
    On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański wrote:
    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
    Alex Hunsaker is listed as the reviewer for this patch, but I don't
    see a review posted.  If this feature is still wanted for 9.1, can
    someone jump in here?
    Goodness... I picked up this patch the day before yesterday because
    no-one was listed. That being said, if anyone else wants to beat me to
    the punch of reviewing this, have at it! The more eyes the merrier!

    I wish I could squeeze
    the lime of my time to find
    a few more hours
  • Robert Haas at Feb 11, 2011 at 4:38 pm

    On Fri, Feb 11, 2011 at 11:22 AM, Alex Hunsaker wrote:
    On Fri, Feb 11, 2011 at 08:45, Robert Haas wrote:
    On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański wrote:
    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
    Alex Hunsaker is listed as the reviewer for this patch, but I don't
    see a review posted.  If this feature is still wanted for 9.1, can
    someone jump in here?
    Goodness... I picked up this patch the day before yesterday because
    no-one was listed. That being said, if anyone else wants to beat me to
    the punch of reviewing this, have at it! The more eyes the merrier!
    Sorry, I didn't see when you'd picked it up. I was just keeping an
    eye on my wall calendar.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Kevin Grittner at Feb 11, 2011 at 5:07 pm

    Robert Haas wrote:

    Goodness... I picked up this patch the day before yesterday
    because no-one was listed. That being said, if anyone else wants
    to beat me to the punch of reviewing this, have at it! The more
    eyes the merrier!
    Sorry, I didn't see when you'd picked it up. I was just keeping
    an eye on my wall calendar.
    [OT]

    FWIW, this is the sort of situation which caused me to suggest that
    the web app somehow show the date of the last reviewer change when
    it is past the "Last Activity" date. I don't really care whether it
    would be in the Reviewers column or as a second line, in
    parentheses, in the Last Activity column. I would find it useful
    when managing a CF, anyway....

    -Kevin
  • Alex Hunsaker at Feb 12, 2011 at 3:12 am

    On Wed, Feb 9, 2011 at 02:10, Jan Urbański wrote:
    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
    In PLy_traceback fname and prname look like they will leak (well as
    much as a palloc() in an error path can leak I suppose). Other than
    that everything looks good. I tested plpython2 and plpython3 and
    skimmed the docs to see if there was anything obvious that needed
    updating. I also obviously looked at the added regression tests and
    made sure they worked.

    Marking as Ready.
  • Jan Urbański at Feb 12, 2011 at 8:48 am

    On 12/02/11 04:12, Alex Hunsaker wrote:
    On Wed, Feb 9, 2011 at 02:10, Jan Urbański wrote:
    On 06/02/11 20:12, Jan Urbański wrote:
    On 27/01/11 22:58, Jan Urbański wrote:
    On 23/12/10 14:56, Jan Urbański wrote:
    Here's a patch implementing traceback support for PL/Python mentioned in
    http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
    an incremental patch on top of the plpython-refactor patch sent eariler.
    Updated to master.
    Updated to master again.
    Once more.
    In PLy_traceback fname and prname look like they will leak (well as
    much as a palloc() in an error path can leak I suppose).
    But they're no palloc'd, no? fname is either a static "<module"> string,
    or PyString_AsString, which also doesn't allocate memory, AFAIK. proname
    is also a static string. They're transferred to heap-allocated memory in
    appendStringInfo, which gets pfreed after emitting the error message.
    Marking as Ready.
    Thanks!

    Jan
  • Alex Hunsaker at Feb 12, 2011 at 9:00 am

    On Sat, Feb 12, 2011 at 01:50, Jan Urbański wrote:
    On 12/02/11 04:12, Alex Hunsaker wrote:
    In PLy_traceback fname and prname look like they will leak (well as
    much as a palloc() in an error path can leak I suppose).
    But they're no palloc'd, no? fname is either a static "<module"> string,
    or PyString_AsString, which also doesn't allocate memory, AFAIK.
    Yeah, I was flat out wrong about proname :-(.

    As for fname, I must be missing some magic. We have:

    #if PY_MAJOR_VERSION > 3
    ...
    #define PyString_AsString(x) PLyUnicode_AsString(x)
    ....
    PLyUnicode_AsString(PyObject *unicode)
    {
    PyObject *o = PLyUnicode_Bytes(unicode);
    char *rv = pstrdup(PyBytes_AsString(o));

    Py_XDECREF(o);
    return rv;
    }

    PyString_AsString is used all over the place without any pfrees. But I
    have no Idea how that pstrdup() is getting freed if at all.

    Care to enlighten me ?
  • Jan Urbański at Feb 12, 2011 at 9:05 am

    On 12/02/11 10:00, Alex Hunsaker wrote:
    On Sat, Feb 12, 2011 at 01:50, Jan Urbański wrote:
    On 12/02/11 04:12, Alex Hunsaker wrote:
    In PLy_traceback fname and prname look like they will leak (well as
    much as a palloc() in an error path can leak I suppose).
    But they're no palloc'd, no? fname is either a static "<module"> string,
    or PyString_AsString, which also doesn't allocate memory, AFAIK.
    Yeah, I was flat out wrong about proname :-(.

    As for fname, I must be missing some magic. We have:

    #if PY_MAJOR_VERSION > 3
    ...
    #define PyString_AsString(x) PLyUnicode_AsString(x)
    ....
    PLyUnicode_AsString(PyObject *unicode)
    {
    PyObject *o = PLyUnicode_Bytes(unicode);
    char *rv = pstrdup(PyBytes_AsString(o));

    Py_XDECREF(o);
    return rv;
    }

    PyString_AsString is used all over the place without any pfrees. But I
    have no Idea how that pstrdup() is getting freed if at all.

    Care to enlighten me ?
    Ooops, seems that this hack that's meant to improve compatibility with
    Python3 makes it leak. I wonder is the pstrdup is necessary here, but
    OTOH the leak should not be overly significant, given that no-one
    complained about it before... and PyString_AsString is being used in
    several other places.

    Jan
  • Peter Eisentraut at Feb 24, 2011 at 12:49 pm

    On lör, 2011-02-12 at 10:07 +0100, Jan Urbański wrote:
    PLyUnicode_AsString(PyObject *unicode)
    {
    PyObject *o = PLyUnicode_Bytes(unicode);
    char *rv = pstrdup(PyBytes_AsString(o));

    Py_XDECREF(o);
    return rv;
    }

    PyString_AsString is used all over the place without any pfrees. But I
    have no Idea how that pstrdup() is getting freed if at all.

    Care to enlighten me ?
    Ooops, seems that this hack that's meant to improve compatibility with
    Python3 makes it leak. I wonder is the pstrdup is necessary here,
    The result of PyBytes_AsString(o) points into the internal storage of o,
    which is released (effectively freed) by the decref on the next line.
    So you'd better make a copy if you want to keep using it.
  • Peter Eisentraut at Feb 24, 2011 at 12:46 pm

    On lör, 2011-02-12 at 02:00 -0700, Alex Hunsaker wrote:
    PyString_AsString is used all over the place without any pfrees. But I
    have no Idea how that pstrdup() is getting freed if at all.
    pstrdup() like palloc() allocates memory from the current memory
    context, which is freed automatically at some useful time, often at the
    end of the query. It is very common throughout the PostgreSQL code that
    memory is not explicitly freed. See src/backend/utils/mmgr/README for
    more information.
  • Peter Eisentraut at Feb 24, 2011 at 1:10 pm

    On tor, 2010-12-23 at 14:56 +0100, Jan Urbański wrote:
    For errors originating from Python exceptions add the traceback as the
    message detail. The patch tries to mimick Python's traceback.py module
    behaviour as close as possible, icluding interleaving stack frames
    with source code lines in the detail message. Any Python developer
    should instantly recognize these kind of error reporting, it looks
    almost the same as an error in the interactive Python shell.
    I think the traceback should go into the CONTEXT part of the error. The
    context message that's already there is now redundant with the
    traceback.

    You could even call errcontext() multiple times to build up the
    traceback, but maybe that's not necessary.
  • Jan Urbański at Feb 24, 2011 at 2:02 pm

    On 24/02/11 14:10, Peter Eisentraut wrote:
    On tor, 2010-12-23 at 14:56 +0100, Jan Urbański wrote:
    For errors originating from Python exceptions add the traceback as the
    message detail. The patch tries to mimick Python's traceback.py module
    behaviour as close as possible, icluding interleaving stack frames
    with source code lines in the detail message. Any Python developer
    should instantly recognize these kind of error reporting, it looks
    almost the same as an error in the interactive Python shell.
    I think the traceback should go into the CONTEXT part of the error. The
    context message that's already there is now redundant with the
    traceback.

    You could even call errcontext() multiple times to build up the
    traceback, but maybe that's not necessary.
    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?

    Jan
  • Robert Haas at Feb 26, 2011 at 5:11 am

    On Thu, Feb 24, 2011 at 9:03 AM, Jan Urbański wrote:
    On 24/02/11 14:10, Peter Eisentraut wrote:
    On tor, 2010-12-23 at 14:56 +0100, Jan Urbański wrote:
    For errors originating from Python exceptions add the traceback as the
    message detail. The patch tries to mimick Python's traceback.py module
    behaviour as close as possible, icluding interleaving stack frames
    with source code lines in the detail message. Any Python developer
    should instantly recognize these kind of error reporting, it looks
    almost the same as an error in the interactive Python shell.
    I think the traceback should go into the CONTEXT part of the error.  The
    context message that's already there is now redundant with the
    traceback.

    You could even call errcontext() multiple times to build up the
    traceback, but maybe that's not necessary.
    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.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Jan Urbański at Feb 26, 2011 at 8:34 am
    ----- Original message -----
    On Thu, Feb 24, 2011 at 9:03 AM, Jan Urbański 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.

    Jan
  • Peter Eisentraut at Feb 26, 2011 at 3:10 pm

    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@wulczer.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.
  • Jan Urbański at Mar 1, 2011 at 8:46 am

    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@wulczer.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
  • Tom Lane at Mar 1, 2011 at 7:15 pm

    =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
    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.
    To me, none of those arguments seem good. Traceback is the sort of
    thing that belongs in errcontext, and arbitarily deciding that plpython
    isn't going to play by the rules doesn't sit well here. I agree that
    what you are showing is redundant with the current errcontext printout,
    but the solution for that is to change the errcontext printout, not to
    add redundant and inappropriate errdetail.

    An example of the reasoning for this is the situation where a plpython
    function calls back into SQL, and something there throws an ereport
    (which might include an errdetail). It would be useful to include the
    Python traceback in the errcontext stack, since there might be multiple
    levels of Python function call within what PG sees as just a "plpython
    function". But you can't get there with this approach.

    regards, tom lane
  • Jan Urbański at Mar 1, 2011 at 7:19 pm

    On 01/03/11 20:15, Tom Lane wrote:
    =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
    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.
    To me, none of those arguments seem good. Traceback is the sort of
    thing that belongs in errcontext, and arbitarily deciding that plpython
    isn't going to play by the rules doesn't sit well here. I agree that
    what you are showing is redundant with the current errcontext printout,
    but the solution for that is to change the errcontext printout, not to
    add redundant and inappropriate errdetail.

    An example of the reasoning for this is the situation where a plpython
    function calls back into SQL, and something there throws an ereport
    (which might include an errdetail). It would be useful to include the
    Python traceback in the errcontext stack, since there might be multiple
    levels of Python function call within what PG sees as just a "plpython
    function". But you can't get there with this approach.
    Currently the traceback is added to the detail and the original
    errdetail is preserved. So you'd get the detail line and the traceback
    below it.

    But OK, since there are more voices in favour of putting tracebacks in
    the context field, I'll keep on looking for a solution.

    Jan
  • Tom Lane at Mar 1, 2011 at 7:36 pm

    =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
    Currently the traceback is added to the detail and the original
    errdetail is preserved. So you'd get the detail line and the traceback
    below it.
    Hm? I'm talking about plpython_error_callback() and friends, which
    AFAICS you haven't changed the behavior of at all. And it would
    certainly be completely inappropriate to do what's said above for
    an errdetail with a non-plpython origin.

    regards, tom lane
  • Jan Urbański at Mar 1, 2011 at 8:09 pm

    On 01/03/11 20:35, Tom Lane wrote:
    =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
    Currently the traceback is added to the detail and the original
    errdetail is preserved. So you'd get the detail line and the traceback
    below it.
    Hm? I'm talking about plpython_error_callback() and friends, which
    AFAICS you haven't changed the behavior of at all. And it would
    certainly be completely inappropriate to do what's said above for
    an errdetail with a non-plpython origin.
    Not sure if I understand the problem. PL/Python sets
    plpython_error_callback right after entering the function call handler,
    so any elog thrown while the function is executing has a "PL/Python
    function %s" context message. If plpython calls into SQL with SPI and
    something there throws an elog(ERROR) with an errdetail, that detail is
    saved inside the exception and a Python error is then thrown. If this
    Python error reaches the top of the Python stack, the error reporting
    code kicks in, extracts the saved errdetail value from the Python
    exception, and then extract the stack trace and also adds it to the
    errdetail.

    So you end up with a context message saying "PL/Python function %s" and
    a detail message with the saved detail (if it's present) *and* the
    traceback. The problem is that the name of the function is already in
    the traceback, so there's no need for the context *if* there's a
    traceback present.

    The problem I'm having is technical: since the callback is already set
    when the code reaches the traceback-printing stage, you can't really
    unset it. AFAICS the elog code calls *all* callbacks from
    error_context_stack. So I can't prevent the context message from
    appearing. If I make the traceback part of the context as well, it's
    just going to appear together with the message from the callback.

    Jan
  • James William Pye at Mar 1, 2011 at 8:24 pm

    On Mar 1, 2011, at 12:10 PM, Jan Urbański wrote:
    So you end up with a context message saying "PL/Python function %s" and
    a detail message with the saved detail (if it's present) *and* the
    traceback. The problem is that the name of the function is already in
    the traceback, so there's no need for the context *if* there's a
    traceback present.

    The problem I'm having is technical: since the callback is already set
    when the code reaches the traceback-printing stage, you can't really
    unset it. AFAICS the elog code calls *all* callbacks from
    error_context_stack. So I can't prevent the context message from
    appearing. If I make the traceback part of the context as well, it's
    just going to appear together with the message from the callback.
    I remember going through a lot of pain getting this done "right"
    in pg-python[pl/py].

    SELECT it_blows_up();
    ERROR: function's "main" raised a Python exception
    CONTEXT: [exception from Python]
    Traceback (most recent call last):
    File "public.it_blows_up()", line 13, in main
    three()
    File "public.it_blows_up()", line 10, in three
    return two()
    File "public.it_blows_up()", line 7, in two
    return one()
    File "public.it_blows_up()", line 4, in one
    raise OverflowError("there's water everywhere")
    OverflowError: there's water everywhere

    [public.it_blows_up()]


    IIRC, I unconditionally print the "[public.it_blows_up()]" part iff it's
    not an ERROR. If it is an ERROR, I let the traceback rendering part of the code
    handle it on the PL's entry point exit. It was really tricky to do this because I
    was rendering the traceback *after* the error_context_stack had been called.
  • Peter Eisentraut at Mar 1, 2011 at 9:14 pm

    On tis, 2011-03-01 at 21:10 +0100, Jan Urbański wrote:
    So you end up with a context message saying "PL/Python function %s"
    and a detail message with the saved detail (if it's present) *and* the
    traceback. The problem is that the name of the function is already in
    the traceback, so there's no need for the context *if* there's a
    traceback present.
    I wouldn't actually worry about that bit of redundancy so much. Getting
    proper context for nested calls is much more important.
  • Jan Urbański at Mar 2, 2011 at 9:27 pm

    On 01/03/11 22:12, Peter Eisentraut wrote:
    On tis, 2011-03-01 at 21:10 +0100, Jan Urbański wrote:
    So you end up with a context message saying "PL/Python function %s"
    and a detail message with the saved detail (if it's present) *and* the
    traceback. The problem is that the name of the function is already in
    the traceback, so there's no need for the context *if* there's a
    traceback present.
    I wouldn't actually worry about that bit of redundancy so much. Getting
    proper context for nested calls is much more important.
    Here's another version that puts tracebacks in the context field.

    I did some tests with the attached test script, calling various of the
    functions defined there and the error messages more or less made sense
    (or at least were not worse than before).

    Cheers,
    Jan
  • Jan Urbański at Mar 6, 2011 at 12:13 pm

    On 02/03/11 22:28, Jan Urbański wrote:
    On 01/03/11 22:12, Peter Eisentraut wrote:
    On tis, 2011-03-01 at 21:10 +0100, Jan Urbański wrote:
    So you end up with a context message saying "PL/Python function %s"
    and a detail message with the saved detail (if it's present) *and* the
    traceback. The problem is that the name of the function is already in
    the traceback, so there's no need for the context *if* there's a
    traceback present.
    I wouldn't actually worry about that bit of redundancy so much. Getting
    proper context for nested calls is much more important.
    Here's another version that puts tracebacks in the context field.

    I did some tests with the attached test script, calling various of the
    functions defined there and the error messages more or less made sense
    (or at least were not worse than before).
    I realized I did not update the patch state in the CF app when I added
    this version, so I flipped it back to Ready for Committer now.

    Tracebacks are a nice-to-have, so if we decide to drop this one due to
    time constraints, I'd understand that. But fixing "raise plpy.Fatal()"
    to actually cause a FATAL is something that should be extracted from
    this patch and committed, even if the full patch does not make it.

    Cheers,
    Jan
  • Peter Eisentraut at Mar 7, 2011 at 12:53 pm

    On sön, 2011-03-06 at 13:14 +0100, Jan Urbański wrote:
    But fixing "raise plpy.Fatal()"
    to actually cause a FATAL is something that should be extracted from
    this patch and committed, even if the full patch does not make it.
    Um, what? I didn't find any details about this in this thread, nor a
    test case.
  • Jan Urbański at Mar 7, 2011 at 1:00 pm

    On 07/03/11 13:53, Peter Eisentraut wrote:
    On sön, 2011-03-06 at 13:14 +0100, Jan Urbański wrote:
    But fixing "raise plpy.Fatal()"
    to actually cause a FATAL is something that should be extracted from
    this patch and committed, even if the full patch does not make it.
    Um, what? I didn't find any details about this in this thread, nor a
    test case.
    Yes, my fault for sneaking it here without more introduction than this
    comment several messages upthread:

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

    So this in fact are three separate things, tracebacks, fix for
    plpy.Fatal and a one-line fix for reporting errors in Python iterators,
    that as I noticed has a side effect of changing the SQLCODE being raised
    :( I think I'll just respin the tracebacks patch as 3 separate ones,
    coming right up.

    BTW, it's hard to test if raising plpy.Fatal actually causes a FATAL
    elog, because that would terminate the backend running the tests, and I
    though pg_regress treats this as an unconditional error (or am I mistaken?).

    Jan
  • Jan Urbański at Mar 7, 2011 at 1:18 pm
  • Peter Eisentraut at Mar 7, 2011 at 9:56 pm

    On mån, 2011-03-07 at 14:19 +0100, Jan Urbański wrote:
    On 07/03/11 14:01, Jan Urbański wrote:
    On 07/03/11 13:53, Peter Eisentraut wrote:
    On sön, 2011-03-06 at 13:14 +0100, Jan Urbański wrote:
    But fixing "raise plpy.Fatal()"
    to actually cause a FATAL is something that should be extracted from
    this patch and committed, even if the full patch does not make it.
    Um, what? I didn't find any details about this in this thread, nor a
    test case.
    So this in fact are three separate things, tracebacks, fix for
    plpy.Fatal and a one-line fix for reporting errors in Python iterators,
    that as I noticed has a side effect of changing the SQLCODE being raised
    :( I think I'll just respin the tracebacks patch as 3 separate ones,
    coming right up.
    Respun as three separate patches. Sorry for the confusion. BTW: looks
    like plpy.Fatal behaviour has been broken for quite some time now.
    Committed 1 and 2.

    Your traceback implementation in PLy_elog is now using two errdetail
    calls in one ereport call, which doesn't work (first one wins). Please
    reconsider that. Also, the comment still talks about the traceback
    going into detail.
  • Jan Urbański at Mar 8, 2011 at 10:09 am

    On 07/03/11 22:55, Peter Eisentraut wrote:
    On mån, 2011-03-07 at 14:19 +0100, Jan Urbański wrote:
    On 07/03/11 14:01, Jan Urbański wrote:
    On 07/03/11 13:53, Peter Eisentraut wrote:
    On sön, 2011-03-06 at 13:14 +0100, Jan Urbański wrote:
    But fixing "raise plpy.Fatal()"
    to actually cause a FATAL is something that should be extracted from
    this patch and committed, even if the full patch does not make it.
    Um, what? I didn't find any details about this in this thread, nor a
    test case.
    So this in fact are three separate things, tracebacks, fix for
    plpy.Fatal and a one-line fix for reporting errors in Python iterators,
    that as I noticed has a side effect of changing the SQLCODE being raised
    :( I think I'll just respin the tracebacks patch as 3 separate ones,
    coming right up.
    Respun as three separate patches. Sorry for the confusion. BTW: looks
    like plpy.Fatal behaviour has been broken for quite some time now.
    Committed 1 and 2.

    Your traceback implementation in PLy_elog is now using two errdetail
    calls in one ereport call, which doesn't work (first one wins). Please
    reconsider that. Also, the comment still talks about the traceback
    going into detail.
    Gah, will look at this and fix.

    Jan
  • Peter Eisentraut at Mar 7, 2011 at 12:53 pm

    On ons, 2011-03-02 at 22:28 +0100, Jan Urbański wrote:
    I did some tests with the attached test script, calling various of the
    functions defined there and the error messages more or less made sense
    (or at least were not worse than before).
    Is that script part of the regression tests you added?
  • Jan Urbański at Mar 7, 2011 at 12:54 pm

    On 07/03/11 13:53, Peter Eisentraut wrote:
    On ons, 2011-03-02 at 22:28 +0100, Jan Urbański wrote:
    I did some tests with the attached test script, calling various of the
    functions defined there and the error messages more or less made sense
    (or at least were not worse than before).
    Is that script part of the regression tests you added?
    No, the regression tests are a bit different. Maybe this script should
    be part of them as well?

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedDec 23, '10 at 1:55p
activeMar 8, '11 at 10:09a
posts35
users7
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase