Обсуждение: Problem returning strings with pgsql 8.3.x
Having posted this to -general [1] per -hackers list instructions [2] to try elsewhere first, and waited (not very long, I admit) in vain for a response, I'm posting this to -hackers now. My apologies if my impatience in that regard annoys. While developing PL/LOLCODE, I've found something wrong with returning strings from LOLCODE functions using 8.3.0 or greater. Using 8.4beta from a few days ago, for instance, a function that should return "test string" returns "\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F" in pgsql (sometimes the number of \x7F characters varies). In 8.2.4 it works fine. Here's the code involved, from pl_lolcode_call_handler, the call handler function for PL/LOLCODE. First, the bit that finds the FmgrInfo structure and typioparam for the result type: procTup = SearchSysCache(PROCOID, ObjectIdGetDatum(fcinfo->flinfo->fn_oid), 0, 0, 0); if (!HeapTupleIsValid(procTup)) elog(ERROR, "Cache lookup failed for procedure %u", fcinfo->flinfo->fn_oid); procStruct = (Form_pg_proc) GETSTRUCT(procTup); typeTup = SearchSysCache(TYPEOID, ObjectIdGetDatum(procStruct->prorettype), 0, 0, 0); if (!HeapTupleIsValid(typeTup)) elog(ERROR, "Cache lookup failed for type %u", procStruct->prorettype); typeStruct = (Form_pg_type) GETSTRUCT(typeTup); resultTypeIOParam = getTypeIOParam(typeTup); fmgr_info_cxt(typeStruct->typinput, &flinfo, TopMemoryContext); /*CurTransactionContext); */ ReleaseSysCache(typeTup); Here's the code that converts the return value into a Datum later on in the function: if (returnTypeOID != VOIDOID) { if (returnVal != NULL) { if (returnVal->type ==ident_NOOB) fcinfo->isnull = true; else { SPI_push(); if (returnTypeOID == BOOLOID) retval= InputFunctionCall(&flinfo, lolVarGetTroof(returnVal) == lolWIN ? "TRUE" : "FALSE", resultTypeIOParam, -1); else { /* elog(NOTICE, lolVarGetString(returnVal, true)); */ retval = InputFunctionCall(&flinfo, lolVarGetString(returnVal, true),resultTypeIOParam, -1); } SPI_pop(); } } else { fcinfo->isnull= true; } } SPI_finish(); /* elog(NOTICE, "PL/LOLCODE ending"); */ return retval; returnVal is an instance of the struct PL/LOLCODE uses to store its variables. The key line in this case is the one after the commented-out call to elog. retval is a Datum type. lolVarGetString() returns the string value the returnVal struct represents -- I'm certain of that thanks to gdb and other testing. All other data types PL/LOLCODE knows about internally seem to return just fine. I'm fairly certain I'm screwing up memory somewhere, but I can't see what I've done wrong. I'm glad to provide further details, but those included above are all the ones I thought were relevant. Thanks in advance for any help you can provide. - Josh / eggyknap [1] http://archives.postgresql.org/pgsql-general/2008-05/msg00311.php [2] http://archives.postgresql.org/pgsql-hackers/
On Mon, May 12, 2008 at 11:23:17PM -0600, Josh Tolley wrote: > SPI_push(); > retval = > InputFunctionCall(&flinfo, lolVarGetString(returnVal, true), > resultTypeIOParam, -1); > SPI_pop(); Won't this cause the return value to be allocated inside a new memory block which gets freeds at the SPI_pop? Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines.
Martijn van Oosterhout <kleptog@svana.org> writes: > On Mon, May 12, 2008 at 11:23:17PM -0600, Josh Tolley wrote: >> SPI_push(); >> retval = >> InputFunctionCall(&flinfo, lolVarGetString(returnVal, true), >> resultTypeIOParam, -1); >> SPI_pop(); > Won't this cause the return value to be allocated inside a new memory > block which gets freeds at the SPI_pop? The SPI_pop in itself is harmless ... the problem is the SPI_finish further down, which will release all simple palloc's done within the SPI function context. What he needs is something comparable to this bit in plpgsql: /* * If the function's return type isn't by value, copy the value * into upper executormemory context. */ if (!fcinfo->isnull && !func->fn_retbyval) { Size len; void *tmp; len = datumGetSize(estate.retval, false, func->fn_rettyplen); tmp = SPI_palloc(len); memcpy(tmp, DatumGetPointer(estate.retval), len); estate.retval = PointerGetDatum(tmp); } ie, push the data into something allocated with SPI_palloc(). I would bet large amounts of money that the problem is not "new in 8.3.0", either. Perhaps Josh was not testing in an --enable-cassert (CLOBBER_FREED_MEMORY) build before. regards, tom lane
On Tue, May 13, 2008 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Martijn van Oosterhout <kleptog@svana.org> writes: > > On Mon, May 12, 2008 at 11:23:17PM -0600, Josh Tolley wrote: > >> SPI_push(); > >> retval = > >> InputFunctionCall(&flinfo, lolVarGetString(returnVal, true), > >> resultTypeIOParam, -1); > >> SPI_pop(); > > > Won't this cause the return value to be allocated inside a new memory > > block which gets freeds at the SPI_pop? > > The SPI_pop in itself is harmless ... the problem is the SPI_finish > further down, which will release all simple palloc's done within the > SPI function context. What he needs is something comparable to this bit > in plpgsql: > > /* > * If the function's return type isn't by value, copy the value > * into upper executor memory context. > */ > if (!fcinfo->isnull && !func->fn_retbyval) > { > Size len; > void *tmp; > > len = datumGetSize(estate.retval, false, func->fn_rettyplen); > tmp = SPI_palloc(len); > memcpy(tmp, DatumGetPointer(estate.retval), len); > estate.retval = PointerGetDatum(tmp); > } > > ie, push the data into something allocated with SPI_palloc(). I'll give this a shot as soon as I can... many thanks > I would bet large amounts of money that the problem is not "new in > 8.3.0", either. Perhaps Josh was not testing in an --enable-cassert > (CLOBBER_FREED_MEMORY) build before. I'll check... that's definitely not unlikely. Again, thanks. - Josh
On Tue, May 13, 2008 at 8:19 AM, Josh Tolley <eggyknap@gmail.com> wrote: > On Tue, May 13, 2008 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Martijn van Oosterhout <kleptog@svana.org> writes: > > > On Mon, May 12, 2008 at 11:23:17PM -0600, Josh Tolley wrote: > > >> SPI_push(); > > >> retval = > > >> InputFunctionCall(&flinfo, lolVarGetString(returnVal, true), > > >> resultTypeIOParam, -1); > > >> SPI_pop(); > > > > > Won't this cause the return value to be allocated inside a new memory > > > block which gets freeds at the SPI_pop? > > > > The SPI_pop in itself is harmless ... the problem is the SPI_finish > > further down, which will release all simple palloc's done within the > > SPI function context. What he needs is something comparable to this bit > > in plpgsql: > > > > /* > > * If the function's return type isn't by value, copy the value > > * into upper executor memory context. > > */ > > if (!fcinfo->isnull && !func->fn_retbyval) > > { > > Size len; > > void *tmp; > > > > len = datumGetSize(estate.retval, false, func->fn_rettyplen); > > tmp = SPI_palloc(len); > > memcpy(tmp, DatumGetPointer(estate.retval), len); > > estate.retval = PointerGetDatum(tmp); > > } > > > > ie, push the data into something allocated with SPI_palloc(). > > I'll give this a shot as soon as I can... many thanks > > > > I would bet large amounts of money that the problem is not "new in > > 8.3.0", either. Perhaps Josh was not testing in an --enable-cassert > > (CLOBBER_FREED_MEMORY) build before. > > I'll check... that's definitely not unlikely. Again, thanks. > > - Josh > Proper (I hope) use of SPI_palloc() took care of this. And yes, the 8.2.x version I was using without problem was compiled without enable-cassert. Once again, thanks. - Josh / eggyknap