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

On 26/03/18 19:07, Nikita Glukhov wrote:
> Attached fixed 3th version of the patch:

Thanks, I'm reviewing this now. Nice speedup!

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.

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.

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?

- Heikki