From: | Haozhou Wang <hawang(at)pivotal(dot)io> |
---|---|
To: | hlinnaka(at)iki(dot)fi |
Cc: | Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>, David Steele <david(at)pgmasters(dot)net>, Anthony Bykov <a(dot)bykov(at)postgrespro(dot)ru>, 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 08:12:36 |
Message-ID: | CAL_NLpLQKUyQMGE9WPDrS8enTcLm7J5cMtp-96eMS9LTThEnfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Heikki,
Thank you very much for your review!
I will prepare a new patch and make it ready soon.
Regards,
Haozhou
On Thu, Jul 12, 2018 at 2:03 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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!
>
> 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
>
--
Regards,
Haozhou
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2018-07-12 08:26:30 | Re: Negotiating the SCRAM channel binding type |
Previous Message | samuel.coulee | 2018-07-12 08:08:44 | Binary difference in pg_internal.init after running pg_initdb multiple times |