Re: plpython tracebacks

I have been working with plpython for several months and have
been hampered by the lack of a traceback being logged when a
plpython function raises an error. I have written a patch causes
the PLy_traceback function to fully log the traceback. The
output looks just like the traceback output provided by the
python interpreter.

Feedback appreciated.

Scott

--- plpython-1.70.c.orig 2006-02-06 14:24:42.000000000 -0600
+++ plpython-1.70.c.patched 2006-02-06 15:34:05.000000000 -0600
@@ -2499,7 +2499,8 @@
*vob = NULL;
char *vstr,
*estr,
- *xstr = NULL;
+ *xstr = NULL,
+ *tbstr;

/*
* get the current exception
@@ -2523,6 +2524,82 @@
else
vstr = "Unknown";

+ /* If there is a traceback object, we build a string containing
+ the traceback information. */
+ if (tb != NULL)
+ {
+ PyObject
+ *cur_tb, /* traceback (tb) item being handled */
+ *old_tb, /* holds tb so we can decrement reference to it */
+ *hdr, /* First line of logged output */
+ *tmpl, /* PyString template for the logged tb item */
+ *ftr, /* Last line of logged output */
+ *tb_list, /* Each tb item create a PyString in this list */
+ *ln, /* The line number of the item in the traceback */
+ *frame, /* the tb_frame */
+ *code, /* the f_code this guy has filename and method name*/
+ *fn, /* the filename of the item in the tb */
+ *nm, /* the function/method name of the item in the tb */
+ *args, /* A tuple of the form (fn, ln, nm) */
+ *logline, /* The assembled string with the logged message */
+ *os, /* points to the os module */
+ *sep, /* line separator */
+ *tb_log; /* PyString with the assembled log msg */
+
+ hdr = PyString_FromString("Traceback (most recent call last):");
+ tmpl = PyString_FromString(" File \"%s\", line %s, in %s");
+ ftr = PyString_FromString("");
+
+ tb_list = PyList_New(0); /* create the list of strings */
+ PyList_Append(tb_list, hdr); /* Append the header to the list */
+
+ /* 1st tb is useless; throw it away */
+ cur_tb = PyObject_GetAttrString(tb, "tb_next");
+ while (cur_tb != Py_None)
+ {
+
+ ln = PyObject_GetAttrString(cur_tb, "tb_lineno");
+ frame = PyObject_GetAttrString(cur_tb, "tb_frame");
+ code = PyObject_GetAttrString(frame, "f_code");
+ fn = PyObject_GetAttrString(code, "co_filename");
+ nm = PyObject_GetAttrString(code, "co_name");
+
+ args = Py_BuildValue("(OOO)", fn, ln, nm); /* args tuple */
+ logline = PyString_Format(tmpl, args); /* build logged string */
+ PyList_Append(tb_list, logline); /* append string to list */
+
+ /* decrement references on all our objects */
+ Py_DECREF(logline);
+ Py_DECREF(args);
+ Py_XDECREF(nm);
+ Py_XDECREF(fn);
+ Py_XDECREF(code);
+ Py_XDECREF(frame);
+ Py_XDECREF(ln);
+
+ old_tb = cur_tb;
+ /* get the next traceback item */
+ cur_tb = PyObject_GetAttrString(cur_tb, "tb_next");
+ Py_DECREF(old_tb); /* we're done with old_tb so decref it */
+ }
+ PyList_Append(tb_list, ftr); /* append the log msg footer */
+
+ os = PyImport_ImportModule("os");
+ sep = PyObject_GetAttrString(os, "linesep"); /* get os EOL char */
+ tb_log = _PyString_Join(sep, tb_list); /* create tb log msgs */
+ tbstr = PyString_AsString(tb_log);
+
+ Py_DECREF(tb_log);
+ Py_DECREF(sep);
+ Py_DECREF(os);
+ Py_DECREF(tb_list);
+ Py_DECREF(ftr);
+ Py_DECREF(tmpl);
+ Py_DECREF(hdr);
+ }
+ else
+ tbstr = "No Traceback";
+
/*
* I'm not sure what to do if eob is NULL here -- we can't call PLy_elog
* because that function calls us, so we could end up with infinite
@@ -2530,7 +2607,7 @@
* Assert() be more appropriate?
*/
estr = eob ? PyString_AsString(eob) : "Unknown Exception";
- xstr = PLy_printf("%s: %s", estr, vstr);
+ xstr = PLy_printf("%s%s: %s", tbstr, estr, vstr);

Py_DECREF(eob);
Py_XDECREF(vob);

On Mon, 2006-02-06 at 16:05 -0600, P. Scott DeVos wrote:
> I have been working with plpython for several months and have
> been hampered by the lack of a traceback being logged when a
> plpython function raises an error. I have written a patch causes
> the PLy_traceback function to fully log the traceback. The
> output looks just like the traceback output provided by the
> python interpreter.

Can any PL/Python folks comment on whether they want this patch?

> Feedback appreciated.

Context diffs are preferred. Also, diffs should be against the root of
the source tree, and attached as MIME attachements if possible (it seems
the mailing list software munges the patch somewhat otherwise).

> + hdr = PyString_FromString("Traceback (most recent call last):");
> + tmpl = PyString_FromString(" File \"%s\", line %s, in %s");
> + ftr = PyString_FromString("");
> +
> + tb_list = PyList_New(0); /* create the list of strings */
> + PyList_Append(tb_list, hdr); /* Append the header to the list */

Minor nit: lowercase "Append". Similarly, consistent capitalization for
all the preceding comments in the block (adjacent to the variable
declarations) would be nice.

> estr = eob ? PyString_AsString(eob) : "Unknown Exception";
> - xstr = PLy_printf("%s: %s", estr, vstr);
> + xstr = PLy_printf("%s%s: %s", tbstr, estr, vstr);

tbstr points into storage owned by tb_log, but the reference to tb_log
has already been released.

-Neil

Neil,

I have written a patch causes
> > the PLy_traceback function to fully log the traceback. The
> > output looks just like the traceback output provided by the
> > python interpreter.
>
> Can any PL/Python folks comment on whether they want this patch?
>
> I am just a humble user of PL/Py, but tracebacks are very wellcome. Not
qualified to judge it's code quality I am.

Harald

--
GHUM Harald Massa
persuadere et programmare
Harald Armin Massa
Reinsburgstraße 202b
70197 Stuttgart
0173/9409607

On Sun, Feb 19, 2006 at 08:09:09PM -0500, Neil Conway wrote:
> On Mon, 2006-02-06 at 16:05 -0600, P. Scott DeVos wrote:
> > I have been working with plpython for several months and have
> > been hampered by the lack of a traceback being logged when a
> > plpython function raises an error. I have written a patch causes
> > the PLy_traceback function to fully log the traceback. The
> > output looks just like the traceback output provided by the
> > python interpreter.
>
> Can any PL/Python folks comment on whether they want this patch?

Yes. Absolutely. This is a required feature for any serious Python user.

One of the great things about python is that almost all debugging can be
done with just the excellent standard python tracebacks. Not having them is a
a not only a inconvience, it signals that the implementation is incomplete
in major ways and, unready for real use.

-dg

--
David Gould daveg(at)sonic(dot)net
If simplicity worked, the world would be overrun with insects.

Neil Conway wrote:

>
> Context diffs are preferred. Also, diffs should be against the root of
> the source tree, and attached as MIME attachements if possible (it seems
> the mailing list software munges the patch somewhat otherwise).
>
> Minor nit: lowercase "Append". Similarly, consistent capitalization for
> all the preceding comments in the block (adjacent to the variable
> declarations) would be nice.
>
> tbstr points into storage owned by tb_log, but the reference to tb_log
> has already been released.
>
I'm on it.

P. Scott DeVos wrote:
> I'm on it.

Actually, don't worry about it -- I've made the corrections I had in
mind myself. Attached is a revised patch. On looking closer, I didn't
really like the way the patch accumulated the lines of the traceback:
AFAICS _PyString_Join() is not an "official" Python C API function (it's
not documented, at any rate), and besides it is cleaner and more
efficient to build up the traceback string in a StringInfo rather than
using Python lists and strings.

The attached patch isn't quite finished: "No Traceback" when there is no
traceback information doesn't seem like the best message, I need to
update the regression tests and some comments, etc. But I plan to apply
something similar in substance to the attached patch to HEAD in the next
day or two, barring objections.

-Neil

Neil Conway wrote:

> Actually, don't worry about it -- I've made the corrections I had in
> mind myself. Attached is a revised patch. On looking closer, I didn't
> really like the way the patch accumulated the lines of the traceback:
> AFAICS _PyString_Join() is not an "official" Python C API function (it's
> not documented, at any rate), and besides it is cleaner and more
> efficient to build up the traceback string in a StringInfo rather than
> using Python lists and strings.
>
I like it. You can tell I'm a python programmer, not a C programmer.

Question:

Are you sure this works: "PyString_AsString(lno)"? lno is a python
integer object. Maybe we want, "PyString_AsString(PyObject_Str(lno))"

> The attached patch isn't quite finished: "No Traceback" when there is no
> traceback information doesn't seem like the best message

OK, how about "There is no traceback information"

, I need to
> update the regression tests and some comments, etc. But I plan to apply
> something similar in substance to the attached patch to HEAD in the next
> day or two, barring objections.
>
Thanks for your attention to this!

P. Scott DeVos wrote:
>
>

>
> Are you sure this works: "PyString_AsString(lno)"? lno is a python
> integer object. Maybe we want, "PyString_AsString(PyObject_Str(lno))"
>
Wait, this is not the way to do it because PyObject_Str returns a new
reference. I think you have to assign PyObject_Str(lno) to another
variable so that you can decrement the reference on it. Unless your
code works as it, in which case, never mind...

Scott

Neil, where are we on this patch?

---------------------------------------------------------------------------

Neil Conway wrote:
> P. Scott DeVos wrote:
> > I'm on it.
>
> Actually, don't worry about it -- I've made the corrections I had in
> mind myself. Attached is a revised patch. On looking closer, I didn't
> really like the way the patch accumulated the lines of the traceback:
> AFAICS _PyString_Join() is not an "official" Python C API function (it's
> not documented, at any rate), and besides it is cleaner and more
> efficient to build up the traceback string in a StringInfo rather than
> using Python lists and strings.
>
> The attached patch isn't quite finished: "No Traceback" when there is no
> traceback information doesn't seem like the best message, I need to
> update the regression tests and some comments, etc. But I plan to apply
> something similar in substance to the attached patch to HEAD in the next
> day or two, barring objections.
>
> -Neil

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian said:
> Neil, where are we on this patch?

My apologies for forgetting about this -- there are a few details I need
to finish off in the patch before I'd like to apply it. I'm on vacation at
the moment and not really in a position to write any code, but I'll
definitely be able to finish the patch when I return at the end of May. If
you'd like to apply the patch sooner, anyone is welcome to finish the
patch -- my recollection is that the only outstanding issues are updating
the regression tests, improving some comments, and considering how this
ought to generalize to other PLs.

-Neil

URL added to TODO list.

---------------------------------------------------------------------------

Neil Conway wrote:
> P. Scott DeVos wrote:
> > I'm on it.
>
> Actually, don't worry about it -- I've made the corrections I had in
> mind myself. Attached is a revised patch. On looking closer, I didn't
> really like the way the patch accumulated the lines of the traceback:
> AFAICS _PyString_Join() is not an "official" Python C API function (it's
> not documented, at any rate), and besides it is cleaner and more
> efficient to build up the traceback string in a StringInfo rather than
> using Python lists and strings.
>
> The attached patch isn't quite finished: "No Traceback" when there is no
> traceback information doesn't seem like the best message, I need to
> update the regression tests and some comments, etc. But I plan to apply
> something similar in substance to the attached patch to HEAD in the next
> day or two, barring objections.
>
> -Neil

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +