Re: [HACKERS] pl/python tracebacks v2

2011-04-20 Thread Peter Eisentraut
On Wed, 2011-04-06 at 23:54 +0200, Jan Urbański wrote:
> > Ouch, just today I found a flaw in this, namely that it assumes the
> > lineno from the traceback always refers to the PL/Python function. If
> > you create a PL/Python function that imports some code, runs it, and
> > that code raises an exception, PLy_traceback will get utterly confused.
> > 
> > Working on a fix...
> 
> Here's the fix.

Committed.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pl/python tracebacks v2

2011-04-06 Thread Jan Urbański
On 06/04/11 22:16, Jan Urbański wrote:
> On 06/04/11 21:38, Peter Eisentraut wrote:
>> On mån, 2011-03-21 at 00:40 +0100, Jan Urbański wrote:
>>> I finally got around to updating the PL/Python tracebacks patch. The
>>> other day I was writing some very simple PL/Python code and the lack of
>>> tracebacks is extremely annoying.
>>
>> I tweaked this a bit to make the patch less invasive, and then committed
>> it. :)
> 
> Ouch, just today I found a flaw in this, namely that it assumes the
> lineno from the traceback always refers to the PL/Python function. If
> you create a PL/Python function that imports some code, runs it, and
> that code raises an exception, PLy_traceback will get utterly confused.
> 
> Working on a fix...

Here's the fix.

The actual bug was funny. The traceback code was fetching the file line
from the traceback and trying to get that line from the original source
to print it. But sometimes that line was refering to a different source
file, like when the exception originated from an imported module.

In my testing I accidentally had the error (in a separate module) on
line 2, so the traceback code tried to fetch line 2 of the function,
which was completely whitespace. This can never happen in theory,
because you can't have a frame starting at an all-whitespace line. The
code to get that line was misbehaving and trying to do a malloc(-2),
which in turn was causing an "ERROR invalid memory allocation".

All that is fixed with the attached patch.

Cheers,
Jan

PS: and thanks for committing that in the first place! :)

J
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 9352580..b2333b8 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*** get_source_line(const char *src, int lin
*** 4507,4512 
--- 4507,4520 
  	if (next == NULL)
  		return pstrdup(s);
  
+ 	/*
+ 	 * Sanity check, next < s if the line was all-whitespace, which should
+ 	 * never happen if Python reported an frame created on that line, but
+ 	 * check anyway.
+ 	 */
+ 	if (next < s)
+ 		return NULL;
+ 
  	return pnstrdup(s, next - s);
  }
  
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4603,4608 
--- 4611,4617 
  		PyObject	*volatile code = NULL;
  		PyObject	*volatile name = NULL;
  		PyObject	*volatile lineno = NULL;
+ 		PyObject	*volatile filename = NULL;
  
  		PG_TRY();
  		{
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4621,4626 
--- 4630,4639 
  			name = PyObject_GetAttrString(code, "co_name");
  			if (name == NULL)
  elog(ERROR, "could not get function name from Python code object");
+ 
+ 			filename = PyObject_GetAttrString(code, "co_filename");
+ 			if (filename == NULL)
+ elog(ERROR, "could not get file name from Python code object");
  		}
  		PG_CATCH();
  		{
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4628,4633 
--- 4641,4647 
  			Py_XDECREF(code);
  			Py_XDECREF(name);
  			Py_XDECREF(lineno);
+ 			Py_XDECREF(filename);
  			PG_RE_THROW();
  		}
  		PG_END_TRY();
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4638,4643 
--- 4652,4658 
  			char	*proname;
  			char	*fname;
  			char	*line;
+ 			char	*plain_filename;
  			long	plain_lineno;
  
  			/*
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4651,4656 
--- 4666,4672 
  fname = PyString_AsString(name);
  
  			proname = PLy_procedure_name(PLy_curr_procedure);
+ 			plain_filename = PyString_AsString(filename);
  			plain_lineno = PyInt_AsLong(lineno);
  
  			if (proname == NULL)
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4662,4668 
  	&tbstr, "\n  PL/Python function \"%s\", line %ld, in %s",
  	proname, plain_lineno - 1, fname);
  
! 			if (PLy_curr_procedure)
  			{
  /*
   * If we know the current procedure, append the exact
--- 4678,4686 
  	&tbstr, "\n  PL/Python function \"%s\", line %ld, in %s",
  	proname, plain_lineno - 1, fname);
  
! 			/* the code object was compiled with "" as the filename */
! 			if (PLy_curr_procedure && plain_filename != NULL &&
! strcmp(plain_filename, "") == 0)
  			{
  /*
   * If we know the current procedure, append the exact
*** PLy_traceback(char **xmsg, char **tbmsg,
*** 4670,4676 
   * traceback.py module behavior.  We could store the
   * already line-split source to avoid splitting it
   * every time, but producing a traceback is not the
!  * most important scenario to optimize for.
   */
  line = get_source_line(PLy_curr_procedure->src, plain_lineno);
  if (line)
--- 4688,4696 
   * traceback.py module behavior.  We could store the
   * already line-split source to avoid splitting it
   * every time, but producing a traceback is not the
!  * most important scenario to optimize for.  However,
!  * do not go as far as traceback.py in reading the source
!  * of imported modules.
   */
 

Re: [HACKERS] pl/python tracebacks v2

2011-04-06 Thread Jan Urbański
On 06/04/11 21:38, Peter Eisentraut wrote:
> On mån, 2011-03-21 at 00:40 +0100, Jan Urbański wrote:
>> I finally got around to updating the PL/Python tracebacks patch. The
>> other day I was writing some very simple PL/Python code and the lack of
>> tracebacks is extremely annoying.
> 
> I tweaked this a bit to make the patch less invasive, and then committed
> it. :)

Ouch, just today I found a flaw in this, namely that it assumes the
lineno from the traceback always refers to the PL/Python function. If
you create a PL/Python function that imports some code, runs it, and
that code raises an exception, PLy_traceback will get utterly confused.

Working on a fix...

Jan

PS: obviously it'd be great to have PL/Python traceback support in 9.1,
but I sure hope we'll get some testing in beta for issues like this...

J

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pl/python tracebacks v2

2011-04-06 Thread Peter Eisentraut
On mån, 2011-03-21 at 00:40 +0100, Jan Urbański wrote:
> I finally got around to updating the PL/Python tracebacks patch. The
> other day I was writing some very simple PL/Python code and the lack of
> tracebacks is extremely annoying.

I tweaked this a bit to make the patch less invasive, and then committed
it. :)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pl/python tracebacks v2

2011-03-20 Thread Peter Geoghegan
On 20 March 2011 23:40, Jan Urbański  wrote:
> I'll update the commitfest app for the 2011-Next commitfest, but if
> someone would like to pick this up and include it in the 9.1 PL/Python
> revamp pack, I'd be thrilled.

I would also be thrilled. I definitely share your sense of frustration
about the lack of tracebacks available when writing pl/python.

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers