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