Re: Problem returning strings with pgsql 8.3.x

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

Note: The -hackers list seemed like the place for this post, but its
list description gives instructions to try another list first, hence
the post here.

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(at)svana(dot)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(at)svana(dot)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 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)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(at)gmail(dot)com> wrote:
> On Tue, May 13, 2008 at 8:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Martijn van Oosterhout <kleptog(at)svana(dot)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