Re: [PATCH] Add missing type conversion functions for PL/Python

On 11.07.2018 21:03, Heikki Linnakangas wrote:

> On 26/03/18 19:07, Nikita Glukhov wrote:
>> Attached fixed 3th version of the patch:
>
> Thanks, I'm reviewing this now. Nice speedup!

Thank you for your review.

>
> There is no test coverage for some of the added code. You can get a 
> code coverage report with:
>
> ./configure --enable-coverage ...
> make
> make -C src/pl/plpython check
> make coverage-html
>
> That produces a code coverage report in coverage/index.html. Please 
> look at the coverage of the new functions, and add tests to 
> src/pl/plpython/sql/plpython_types.sql so that all the new code is 
> covered.
>
I have added some cross-type test cases and now almost all new code is covered
(excluding several error cases which can be triggered only by custom numeric
type implementations).


> In some places, where you've already checked the object type e.g. with 
> PyFloat_Check(), I think you could use the less safe variants, like 
> PyFloat_AS_DOUBLE() instead of PyFloat_AsDouble(). Since this patch is 
> all about performance, seems worth doing.

Fixed.


> Some of the conversions seem a bit questionable. For example:
>
>> /*
>>  * Convert a Python object to a PostgreSQL float8 datum directly.
>>  * If can not convert it directly, fallback to PLyObject_ToScalar
>>  * to convert it.
>>  */
>> static Datum
>> PLyObject_ToFloat8(PLyObToDatum *arg, PyObject *plrv,
>>                    bool *isnull, bool inarray)
>> {
>>     if (plrv == Py_None)
>>     {
>>         *isnull = true;
>>         return (Datum) 0;
>>     }
>>
>>     if (PyFloat_Check(plrv) || PyInt_Check(plrv) || PyLong_Check(plrv))
>>     {
>>         *isnull = false;
>>         return Float8GetDatum((double)PyFloat_AsDouble(plrv));
>>     }
>>
>>     return PLyObject_ToScalar(arg, plrv, isnull, inarray);
>> }
>
> The conversion from Python int to C double is performed by 
> PyFloat_AsDouble(). But there's no error checking. And wouldn't 
> PyLong_AsDouble() be more appropriate in that case, since we already 
> checked the python type?
>

Yes, this might be wrong, but PyFloat_AsDouble() internally tries first to
convert number to float.  Also, after gaining more experience in PL/Python
during the implementation of jsonb transforms, I found a lot of similar
problems in the code.  All of them are fixed in the 4th version of the patch.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company