improve type conversion of SPI_processed in Python
Here is a patch to improves how PL/Python deals with very large values
of SPI_processed. The previous code converts anything that does not fit
into a C long into a Python float. But Python long has unlimited
precision, so we should be using that instead. And in Python 3, int and
long as the same, so there is no need to deal with any variations anymore.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Improve-type-conversion-of-SPI_processed-in-Python.patchtext/plain; charset=UTF-8; name=0001-Improve-type-conversion-of-SPI_processed-in-Python.patch; x-mac-creator=0; x-mac-type=0Download
From ba45631d66502600034f3d4803e35909f29d8e7c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Tue, 12 Sep 2017 22:09:21 -0400
Subject: [PATCH] Improve type conversion of SPI_processed in Python
The previous code converted SPI_processed to a Python float if it didn't
fit into a Python int. But Python longs have unlimited precision, so
use that instead.
Refactor the code a bit to avoid having to repeat this logic three times.
---
src/pl/plpython/plpy_cursorobject.c | 4 +---
src/pl/plpython/plpy_spi.c | 8 ++------
src/pl/plpython/plpy_util.c | 25 +++++++++++++++++++++++++
src/pl/plpython/plpy_util.h | 2 ++
4 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 9467f64808..37baf5fafd 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -437,9 +437,7 @@ PLy_cursor_fetch(PyObject *self, PyObject *args)
ret->status = PyInt_FromLong(SPI_OK_FETCH);
Py_DECREF(ret->nrows);
- ret->nrows = (SPI_processed > (uint64) LONG_MAX) ?
- PyFloat_FromDouble((double) SPI_processed) :
- PyInt_FromLong((long) SPI_processed);
+ ret->nrows = PLyObject_FromUint64(SPI_processed);
if (SPI_processed != 0)
{
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 0c623a9458..c6b6f73498 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -371,9 +371,7 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
if (status > 0 && tuptable == NULL)
{
Py_DECREF(result->nrows);
- result->nrows = (rows > (uint64) LONG_MAX) ?
- PyFloat_FromDouble((double) rows) :
- PyInt_FromLong((long) rows);
+ result->nrows = PLyObject_FromUint64(rows);
}
else if (status > 0 && tuptable != NULL)
{
@@ -381,9 +379,7 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
MemoryContext cxt;
Py_DECREF(result->nrows);
- result->nrows = (rows > (uint64) LONG_MAX) ?
- PyFloat_FromDouble((double) rows) :
- PyInt_FromLong((long) rows);
+ result->nrows = PLyObject_FromUint64(rows);
cxt = AllocSetContextCreate(CurrentMemoryContext,
"PL/Python temp context",
diff --git a/src/pl/plpython/plpy_util.c b/src/pl/plpython/plpy_util.c
index 35d57a9e80..0bb5150de9 100644
--- a/src/pl/plpython/plpy_util.c
+++ b/src/pl/plpython/plpy_util.c
@@ -133,3 +133,28 @@ PLyUnicode_FromString(const char *s)
}
#endif /* PY_MAJOR_VERSION >= 3 */
+
+/*
+ * Return a suitable Python object containing a uint64.
+ */
+PyObject *
+PLyObject_FromUint64(uint64 ival)
+{
+#if PY_MAJOR_VERSION < 3
+ /* In Python 2, return int if it fits. */
+ if (ival <= (uint64) LONG_MAX)
+ return PyInt_FromLong((long) ival);
+ else
+#endif
+ {
+ /*
+ * Convert to Python long, picking the conversion function that
+ * corresponds to the underlying definition of our uint64.
+ */
+#ifdef HAVE_LONG_LONG
+ return PyLong_FromUnsignedLongLong(ival);
+#else
+ return PyLong_FromUnsignedLong(ival);
+#endif
+ }
+}
diff --git a/src/pl/plpython/plpy_util.h b/src/pl/plpython/plpy_util.h
index f990bb0890..1c7e109617 100644
--- a/src/pl/plpython/plpy_util.h
+++ b/src/pl/plpython/plpy_util.h
@@ -14,4 +14,6 @@ extern PyObject *PLyUnicode_FromString(const char *s);
extern PyObject *PLyUnicode_FromStringAndSize(const char *s, Py_ssize_t size);
#endif
+extern PyObject *PLyObject_FromUint64(uint64 ival);
+
#endif /* PLPY_UTIL_H */
--
2.15.1
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Here is a patch to improves how PL/Python deals with very large values
of SPI_processed. The previous code converts anything that does not fit
into a C long into a Python float. But Python long has unlimited
precision, so we should be using that instead. And in Python 3, int and
long as the same, so there is no need to deal with any variations anymore.
I took a quick look at this. +1 for returning Python long all the time,
but I wonder why the Python version dependency. Our existing function
PLyLong_FromInt64() believes that PyLong_FromLongLong is unconditionally
available. I'd be inclined to code PLyObject_FromUint64() as an exact
analog of PLyLong_FromInt64(), ie
/* on 32 bit platforms "unsigned long" may be too small */
if (sizeof(uint64) > sizeof(unsigned long))
return PyLong_FromUnsignedLongLong(DatumGetUInt64(d));
else
return PyLong_FromUnsignedLong(DatumGetUInt64(d));
and let Python worry about how to optimize the conversion.
So far as I can tell from
https://docs.python.org/2/c-api/long.html
these functions are available as far back as we could need.
If the buildfarm tells us otherwise, we could deal with it then.
regards, tom lane
On 1/9/18 15:54, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Here is a patch to improves how PL/Python deals with very large values
of SPI_processed. The previous code converts anything that does not fit
into a C long into a Python float. But Python long has unlimited
precision, so we should be using that instead. And in Python 3, int and
long as the same, so there is no need to deal with any variations anymore.I took a quick look at this. +1 for returning Python long all the time,
but I wonder why the Python version dependency.
To keep returning an int in Python 2 in the cases it fits. Maybe that's
overkill.
Our existing function
PLyLong_FromInt64() believes that PyLong_FromLongLong is unconditionally
available.
Interesting. I had coded this to account for the possibility that long
long does not exist on a 64-bit platform, but that situation probably
died with the first Alpha or something.
I'd be inclined to code PLyObject_FromUint64() as an exact
analog of PLyLong_FromInt64(), ie/* on 32 bit platforms "unsigned long" may be too small */
if (sizeof(uint64) > sizeof(unsigned long))
return PyLong_FromUnsignedLongLong(DatumGetUInt64(d));
else
return PyLong_FromUnsignedLong(DatumGetUInt64(d));and let Python worry about how to optimize the conversion.
Would that even be necessary? Why not use the LongLong variant all the
time then?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 1/9/18 15:54, Tom Lane wrote:
I'd be inclined to code PLyObject_FromUint64() as an exact
analog of PLyLong_FromInt64(), ie/* on 32 bit platforms "unsigned long" may be too small */
if (sizeof(uint64) > sizeof(unsigned long))
return PyLong_FromUnsignedLongLong(DatumGetUInt64(d));
else
return PyLong_FromUnsignedLong(DatumGetUInt64(d));and let Python worry about how to optimize the conversion.
Would that even be necessary? Why not use the LongLong variant all the
time then?
I've not looked at the Python internals to see if one is noticeably faster
than the other, but if it isn't, yeah we could simplify that. In any case
my main point is let's keep these two functions using similar approaches.
regards, tom lane
On 1/12/18 11:06, Tom Lane wrote:
Would that even be necessary? Why not use the LongLong variant all the
time then?I've not looked at the Python internals to see if one is noticeably faster
than the other, but if it isn't, yeah we could simplify that. In any case
my main point is let's keep these two functions using similar approaches.
I ran extensive tests on this and went ahead with the simplified versions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services