pl/python tracebacks

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.

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.

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.

On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański <wulczer@wulczer.org> 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

On Fri, Feb 11, 2011 at 08:45, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański <wulczer@wulczer.org> 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

On Fri, Feb 11, 2011 at 11:22 AM, Alex Hunsaker <badalex@gmail.com> wrote:

On Fri, Feb 11, 2011 at 08:45, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Feb 9, 2011 at 4:10 AM, Jan Urbański <wulczer@wulczer.org> 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

Robert Haas <robertmhaas@gmail.com> 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

On Wed, Feb 9, 2011 at 02:10, Jan Urbański <wulczer@wulczer.org> 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.

On 12/02/11 04:12, Alex Hunsaker wrote:

On Wed, Feb 9, 2011 at 02:10, Jan Urbański <wulczer@wulczer.org> 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

On Sat, Feb 12, 2011 at 01:50, Jan Urbański <wulczer@wulczer.org> 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 ?

On 12/02/11 10:00, Alex Hunsaker wrote:

On Sat, Feb 12, 2011 at 01:50, Jan Urbański <wulczer@wulczer.org> 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

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.

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.

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.

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

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:

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

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

Jan

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.

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