Re: [PATCHES] plpython crash on exception

2007-11-23 Thread Marko Kreen
On 11/23/07, Tom Lane [EMAIL PROTECTED] wrote:
 Marko Kreen [EMAIL PROTECTED] writes:
  On 11/23/07, Tom Lane [EMAIL PROTECTED] wrote:
  Why?  I can't imagine any real use for it.  If you're thinking that
  it could provide a guide as to what to resize the buffer to, think
  again.

   If the output was truncated due to this limit then the return
   value is the number of characters (not including the trailing
   '\0') which would have been written to the final string if
   enough space had  been  available.

  What problem do you see?

 The problem is that you are quoting from some particular system's
 manual, and not any kind of standard ... much less any standard that
 every platform we support follows.

 The real-world situation is that we are lucky to be able to tell
 vsnprintf success from failure at all :-(

Ah, ok.

I just saw the result used inside the function, so I thought
it is standard enough.


Actually, the meaning could be changed to *needmore
and compensated inside function:

 *needmore = (nprinted  buf-maxlen) ? buf-maxlen : nprinted + 1;

Then it would not matter if libc is conforming or not.

-- 
marko

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


Re: [PATCHES] plpython crash on exception

2007-11-23 Thread Tom Lane
Marko Kreen [EMAIL PROTECTED] writes:
 Actually, the meaning could be changed to *needmore
 and compensated inside function:

How's that different from the existing function result?

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [PATCHES] plpython crash on exception

2007-11-23 Thread Alvaro Herrera
Marko Kreen escribió:
 On 11/23/07, Alvaro Herrera [EMAIL PROTECTED] wrote:

  (If you are really interested in a fix for 7.3, let me know too ... I
  couldn't even get plpython to run a trivial function due to RExec
  issues.  I didn't try 7.2).
 
 RExec seems to hint that 7.3 can work only with python 2.2
 and below.  What version did you try it on?

I have 2.4.4 here.

 But no, I don't personally care about 7.3...

I must admit I was a bit annoyed at modifying 7.4's patch to fit and
then finding out that I couldn't test it.

-- 
Alvaro Herrerahttp://www.advogato.org/person/alvherre
If it wasn't for my companion, I believe I'd be having
the time of my life  (John Dunbar)

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Tom Lane
Marko Kreen [EMAIL PROTECTED] writes:
 Following function crashes plpython on x86-64 / gcc 4.1.2 / debian 4.0:
   CREATE FUNCTION crashme(str_len integer)
   RETURNS text AS $$
 raise Exception(X * str_len)
   $$ LANGUAGE plpythonu;

   SELECT crashme(1000);

 Problem turns out to be va_list handling in PLy_vprintf() which
 uses same va_list repeatedly.  Fix is to va_copy to temp variable.

This patch isn't acceptable because va_copy() isn't portable.

I'm kinda wondering why PLy_printf and the functions after it even
exist.  They look like rather poorly done reimplementations of
functionality that exists elsewhere in the backend (eg, stringinfo.c).
In particular, why malloc and not palloc?

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Alvaro Herrera
Tom Lane escribió:
 Marko Kreen [EMAIL PROTECTED] writes:
  Following function crashes plpython on x86-64 / gcc 4.1.2 / debian 4.0:
CREATE FUNCTION crashme(str_len integer)
RETURNS text AS $$
  raise Exception(X * str_len)
$$ LANGUAGE plpythonu;
 
SELECT crashme(1000);
 
  Problem turns out to be va_list handling in PLy_vprintf() which
  uses same va_list repeatedly.  Fix is to va_copy to temp variable.
 
 This patch isn't acceptable because va_copy() isn't portable.
 
 I'm kinda wondering why PLy_printf and the functions after it even
 exist.  They look like rather poorly done reimplementations of
 functionality that exists elsewhere in the backend (eg, stringinfo.c).
 In particular, why malloc and not palloc?

See attached patch.

I didn't bother to change the PLy_malloc and friends because I think
that would be too much change for 8.3.  PLy_realloc is gone though
because there are no callers left after this patch.

-- 
Alvaro Herrera http://www.flickr.com/photos/alvherre/
Ninguna manada de bestias tiene una voz tan horrible como la humana (Orual)
Index: src/pl/plpython/plpython.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/pl/plpython/plpython.c,v
retrieving revision 1.104
diff -c -p -r1.104 plpython.c
*** src/pl/plpython/plpython.c	15 Nov 2007 21:14:46 -	1.104
--- src/pl/plpython/plpython.c	22 Nov 2007 18:11:02 -
*** static char *PLy_procedure_name(PLyProce
*** 210,220 
  /* some utility functions */
  static void PLy_elog(int, const char *,...);
  static char *PLy_traceback(int *);
- static char *PLy_vprintf(const char *fmt, va_list ap);
- static char *PLy_printf(const char *fmt,...);
  
  static void *PLy_malloc(size_t);
- static void *PLy_realloc(void *, size_t);
  static char *PLy_strdup(const char *);
  static void PLy_free(void *);
  
--- 210,217 
*** PLy_exception_set(PyObject * exc, const 
*** 2900,2934 
  static void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	va_list		ap;
! 	char	   *xmsg,
! 			   *emsg;
  	int			xlevel;
  
  	xmsg = PLy_traceback(xlevel);
  
! 	va_start(ap, fmt);
! 	emsg = PLy_vprintf(fmt, ap);
! 	va_end(ap);
  
  	PG_TRY();
  	{
  		ereport(elevel,
! (errmsg(plpython: %s, emsg),
   (xmsg) ? errdetail(%s, xmsg) : 0));
  	}
  	PG_CATCH();
  	{
! 		PLy_free(emsg);
  		if (xmsg)
! 			PLy_free(xmsg);
  		PG_RE_THROW();
  	}
  	PG_END_TRY();
  
! 	PLy_free(emsg);
  	if (xmsg)
! 		PLy_free(xmsg);
  }
  
  static char *
--- 2897,2940 
  static void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	char	   *xmsg;
  	int			xlevel;
+ 	StringInfoData emsg;
  
  	xmsg = PLy_traceback(xlevel);
  
! 	initStringInfo(emsg);
! 	for (;;)
! 	{
! 		va_list		ap;
! 		bool		success;
! 
! 		va_start(ap, fmt);
! 		success = appendStringInfoVA(emsg, fmt, ap);
! 		va_end(ap);
! 		if (success)
! 			break;
! 		enlargeStringInfo(emsg, emsg.maxlen);
! 	}
  
  	PG_TRY();
  	{
  		ereport(elevel,
! (errmsg(plpython: %s, emsg.data),
   (xmsg) ? errdetail(%s, xmsg) : 0));
  	}
  	PG_CATCH();
  	{
! 		pfree(emsg.data);
  		if (xmsg)
! 			pfree(xmsg);
  		PG_RE_THROW();
  	}
  	PG_END_TRY();
  
! 	pfree(emsg.data);
  	if (xmsg)
! 		pfree(xmsg);
  }
  
  static char *
*** PLy_traceback(int *xlevel)
*** 2940,2947 
  	PyObject   *eob,
  			   *vob = NULL;
  	char	   *vstr,
! 			   *estr,
! 			   *xstr = NULL;
  
  	/*
  	 * get the current exception
--- 2946,2953 
  	PyObject   *eob,
  			   *vob = NULL;
  	char	   *vstr,
! 			   *estr;
! 	StringInfoData xstr;
  
  	/*
  	 * get the current exception
*** PLy_traceback(int *xlevel)
*** 2973,2979 
  	 * Assert() be more appropriate?
  	 */
  	estr = eob ? PyString_AsString(eob) : Unknown Exception;
! 	xstr = PLy_printf(%s: %s, estr, vstr);
  
  	Py_DECREF(eob);
  	Py_XDECREF(vob);
--- 2979,2986 
  	 * Assert() be more appropriate?
  	 */
  	estr = eob ? PyString_AsString(eob) : Unknown Exception;
! 	initStringInfo(xstr);
! 	appendStringInfo(xstr, %s: %s, estr, vstr);
  
  	Py_DECREF(eob);
  	Py_XDECREF(vob);
*** PLy_traceback(int *xlevel)
*** 2990,3038 
  		*xlevel = ERROR;
  
  	Py_DECREF(e);
! 	return xstr;
! }
! 
! static char *
! PLy_printf(const char *fmt,...)
! {
! 	va_list		ap;
! 	char	   *emsg;
! 
! 	va_start(ap, fmt);
! 	emsg = PLy_vprintf(fmt, ap);
! 	va_end(ap);
! 	return emsg;
! }
! 
! static char *
! PLy_vprintf(const char *fmt, va_list ap)
! {
! 	size_t		blen;
! 	int			bchar,
! tries = 2;
! 	char	   *buf;
! 
! 	blen = strlen(fmt) * 2;
! 	if (blen  256)
! 		blen = 256;
! 	buf = PLy_malloc(blen * sizeof(char));
! 
! 	while (1)
! 	{
! 		bchar = vsnprintf(buf, blen, fmt, ap);
! 		if (bchar  0  bchar  blen)
! 			return buf;
! 		if (tries-- = 0)
! 			break;
! 		if (blen  0)
! 			blen = bchar + 1;
! 		else
! 			blen *= 2;
! 		buf = PLy_realloc(buf, blen);
! 	}
! 	PLy_free(buf);
! 	return NULL;
  }
  
  /* python module 

Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Tom Lane
Alvaro Herrera [EMAIL PROTECTED] writes:
 Tom Lane escribió:
 This patch isn't acceptable because va_copy() isn't portable.
 
 I'm kinda wondering why PLy_printf and the functions after it even
 exist.  They look like rather poorly done reimplementations of
 functionality that exists elsewhere in the backend (eg, stringinfo.c).
 In particular, why malloc and not palloc?

 See attached patch.

 I didn't bother to change the PLy_malloc and friends because I think
 that would be too much change for 8.3.  PLy_realloc is gone though
 because there are no callers left after this patch.

Yeah, that is about what I was thinking too.  Are you set up to
back-patch this as far as 7.3?  If so, please apply.

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Alvaro Herrera
Tom Lane escribió:
 Alvaro Herrera [EMAIL PROTECTED] writes:
  Tom Lane escribi�:
  This patch isn't acceptable because va_copy() isn't portable.
  
  I'm kinda wondering why PLy_printf and the functions after it even
  exist.  They look like rather poorly done reimplementations of
  functionality that exists elsewhere in the backend (eg, stringinfo.c).
  In particular, why malloc and not palloc?
 
  See attached patch.
 
  I didn't bother to change the PLy_malloc and friends because I think
  that would be too much change for 8.3.  PLy_realloc is gone though
  because there are no callers left after this patch.
 
 Yeah, that is about what I was thinking too.  Are you set up to
 back-patch this as far as 7.3?  If so, please apply.

Yeah, I think I'm done with the patch, but the regression tests do not
work from 8.0 back in VPATH builds it seems, so I'll have to rebuild
them to check.

One problem here is that 7.3 does not have appendStringInfoVA.  From 7.4
onwards there is no problem.  Should I backport the code from 7.4?  (On
a first try to do it I introduced a nasty bug, so it's not as easy as
a cut'n paste).

-- 
Alvaro Herrerahttp://www.advogato.org/person/alvherre
Postgres is bloatware by design: it was built to house
 PhD theses. (Joey Hellerstein, SIGMOD annual conference 2002)

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Alvaro Herrera
Alvaro Herrera escribió:

 One problem here is that 7.3 does not have appendStringInfoVA.  From 7.4
 onwards there is no problem.  Should I backport the code from 7.4?  (On
 a first try to do it I introduced a nasty bug, so it's not as easy as
 a cut'n paste).

No nasty bug anywhere -- just that I neglected a initStringInfo call
while manually backporting the patch.

However, in testing I noticed this problem:

alvherre=# select crashme(1);
ERROR:  plpython: Unable to create rexec.RExec instance
exceptions.RuntimeError: This code is not secure in Python 2.2 and 2.3

There doesn't seem to be a way to make it work.

What I'm going to do is commit the fix to just 7.4 onwards.

-- 
Alvaro Herrerahttp://www.advogato.org/person/alvherre
We're here to devour each other alive(Hobbes)

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


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Tom Lane
Alvaro Herrera [EMAIL PROTECTED] writes:
 What I'm going to do is commit the fix to just 7.4 onwards.

Fair enough.  7.3 is on life support anyway --- I'm not in favor of
heroic efforts to port patches that far back.

regards, tom lane

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


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Alvaro Herrera
Marko Kreen escribió:

 Problem turns out to be va_list handling in PLy_vprintf() which
 uses same va_list repeatedly.  Fix is to va_copy to temp variable.

Marko, I just applied what I posted earlier today from 7.4 to 8.2 and
HEAD.  I assume you had a more complex test case; please have a try with
the code in CVS and let me know.

(If you are really interested in a fix for 7.3, let me know too ... I
couldn't even get plpython to run a trivial function due to RExec
issues.  I didn't try 7.2).

-- 
Alvaro Herrera  Developer, http://www.PostgreSQL.org/
Es filósofo el que disfruta con los enigmas (G. Coli)

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Tom Lane
Marko Kreen [EMAIL PROTECTED] writes:
 On 11/23/07, Tom Lane [EMAIL PROTECTED] wrote:
 Why?  I can't imagine any real use for it.  If you're thinking that
 it could provide a guide as to what to resize the buffer to, think
 again.

  If the output was truncated due to this limit then the return
  value is the number of characters (not including the trailing
  '\0') which would have been written to the final string if
  enough space had  been  available.

 What problem do you see?

The problem is that you are quoting from some particular system's
manual, and not any kind of standard ... much less any standard that
every platform we support follows.

The real-world situation is that we are lucky to be able to tell
vsnprintf success from failure at all :-(

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Marko Kreen
On 11/23/07, Tom Lane [EMAIL PROTECTED] wrote:
 Marko Kreen [EMAIL PROTECTED] writes:
  Just a note - appendStringInfoVA should take *nprinted argument.

 Why?  I can't imagine any real use for it.  If you're thinking that
 it could provide a guide as to what to resize the buffer to, think
 again.

It seems perfectly appropriate:

 If the output was truncated due to this limit then the return
 value is the number of characters (not including the trailing
 '\0') which would have been written to the final string if
 enough space had  been  available.

What problem do you see?

Note that I don't want to fix unnecessary memory usage with that
but several reallocs in case some args are large.

-- 
marko

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


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Tom Lane
Marko Kreen [EMAIL PROTECTED] writes:
 Just a note - appendStringInfoVA should take *nprinted argument.

Why?  I can't imagine any real use for it.  If you're thinking that
it could provide a guide as to what to resize the buffer to, think
again.

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Marko Kreen
On 11/22/07, Alvaro Herrera [EMAIL PROTECTED] wrote:
 One problem here is that 7.3 does not have appendStringInfoVA.  From 7.4
 onwards there is no problem.  Should I backport the code from 7.4?  (On
 a first try to do it I introduced a nasty bug, so it's not as easy as
 a cut'n paste).

Just a note - appendStringInfoVA should take *nprinted argument.

Can this be added in 8.4?  Should make usage saner.

-- 
marko

---(end of broadcast)---
TIP 6: explain analyze is your friend


Re: [PATCHES] plpython crash on exception

2007-11-22 Thread Marko Kreen
On 11/23/07, Alvaro Herrera [EMAIL PROTECTED] wrote:
 Marko Kreen escribió:
  Problem turns out to be va_list handling in PLy_vprintf() which
  uses same va_list repeatedly.  Fix is to va_copy to temp variable.

 Marko, I just applied what I posted earlier today from 7.4 to 8.2 and
 HEAD.  I assume you had a more complex test case; please have a try with
 the code in CVS and let me know.

Thanks, I'll test it.

 (If you are really interested in a fix for 7.3, let me know too ... I
 couldn't even get plpython to run a trivial function due to RExec
 issues.  I didn't try 7.2).

RExec seems to hint that 7.3 can work only with python 2.2
and below.  What version did you try it on?

But no, I don't personally care about 7.3...

-- 
marko

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly