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

From: Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
To: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, David Steele <david(at)pgmasters(dot)net>, Haozhou Wang <hawang(at)pivotal(dot)io>, Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)postgresql(dot)org, Paul Guo <pguo(at)pivotal(dot)io>, Hubert Zhang <hzhang(at)pivotal(dot)io>
Subject: Re: [PATCH] Add missing type conversion functions for PL/Python
Date: 2018-07-12 15:06:21
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.


> 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:
The Russian Postgres Company

Attachment Content-Type Size
0001-Add-missing-type-conversion-functions-for-PL-Python-v4.patch text/x-patch 40.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-07-12 15:14:55 Re: Postgres 11 release notes
Previous Message Teodor Sigaev 2018-07-12 15:00:27 Re: cost_sort() improvements