Re: [PATCHES] plpython crash on exception
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
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
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
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
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
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
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
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
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
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
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
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
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
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
