Re: Array of composite types returned from python

Le dimanche 29 juin 2014 16:54:03 Tom Lane a écrit :
> Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com> writes:
> >> 3) This is such a simple change with no new infrastructure code
> >> (PLyObject_ToComposite already exists). Can you think of a reason
> >> why this wasn't done until now? Was it a simple miss or purposefully
> >> excluded?
> >
> > This is not an authoritative answer: I think the infrastructure was
> > originally missing, but was later added in #bc411f25 for OUT parameters.
> > Perhaps it was overlooked at the time that the same code would suffice
> > for this earlier-missing case. (I've Cc:ed Peter E. in case he has any
> > comments.)
> >
> > I think the patch is ready for committer.

Sorry for being this late.

I've tested the patch, everything seems to work as expected, including complex
nesting of Composite and array types.

No documentation changes are needed, since the limitation wasn't even
mentioned before.

Regression tests are ok, and the patch seems simple enough. Formatting looks
OK too.

>
> I took a quick look at this; not really a review either, but I have
> a couple comments.
>
> 1. While I think the patch does what it intends to, it's a bit distressing
> that it will invoke the information lookups in PLyObject_ToComposite over
> again for *each element* of the array. We probably ought to quantify that
> overhead to see if it's bad enough that we need to do something about
> improving caching, as speculated in the comment in PLyObject_ToComposite.

I don't know how to do that without implementing the cache itself.

>
> 2. I wonder whether the no-composites restriction in plpy.prepare
> (see lines 133ff in plpy_spi.c) could be removed as easily.

Hum, I tried that, but its not that easy: lifting the restriction results in a
SEGFAULT when trying to pfree the parameters given to SPI_ExecutePlan (line
320 in plpy_spi.c).

Correct me if I'm wrong, but I think the problem is that HeapTupleGetDatum
returns the t_data field, whereas heap_form_tuple allocation returns the
address of the HeapTuple itself. Then, the datum itself has not been palloced.

Changing the HeapTupleGetDatum call for an heap_copy_tuple_as_datum fixes this
issue, but I'm not sure this the best way to do that.

The attached patch implements this.

>
> regards, tom lane

--
Ronan Dunklau
http://dalibo.com - http://dalibo.org