Error code missing for "wrong length of inner sequence" error
In PLySequence_ToArray_recurse(), there's this check:
if (PySequence_Length(list) != dims[dim])
ereport(ERROR,
(errmsg("wrong length of inner sequence: has length %d, but %d was expected",
(int) PySequence_Length(list), dims[dim]),
(errdetail("To construct a multidimensional array, the inner sequences must all have the same length."))));
It's missing an error code, making it implicitly ERRCODE_INTERNAL_ERROR,
which is surely not right. That's simple to fix, but what error code
should we use?
I'm inclined to use ERRCODE_ARRAY_SUBSCRIPT_ERROR, because that's used
in the similar error message with SQL ARRAY expression, like "ARRAY[[1],
[2, 3]]". Most checks when converting between SQL and Python types use
the PLy_elog() function, which uses the genericc
ERRCODE_EXTERNAL_ROUTINE_EXCEPTION error code, but I think
ERRCODE_ARRAY_SUBSCRIPT_ERROR is better.
You get this error e.g. if you try to return a python lists of lists
from a PL/python function as a multi-dimensional array, but the sublists
have different lengths:
create function return_array() returns int[] as $$
return [[1], [1,2]]
$$ language plpythonu;
postgres=# select return_array();
ERROR: wrong length of inner sequence: has length 2, but 1 was expected
DETAIL: To construct a multidimensional array, the inner sequences must
all have the same length.
CONTEXT: while creating return value
PL/Python function "return_array"
Thoughts? If not, I'm going to add
errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR) to that.
- Heikki
On 1 Oct 2020, at 12:54, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Most checks when converting between SQL and Python types use the PLy_elog() function, which uses the genericc ERRCODE_EXTERNAL_ROUTINE_EXCEPTION error code, but I think ERRCODE_ARRAY_SUBSCRIPT_ERROR is better.
On that note, wouldn't the dimension check errors in PLySequence_ToArray be
just as well off using normal ereport()'s? Only one of them seem to error out
in a way that could propagate an error from Python to postgres.
Thoughts? If not, I'm going to add errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR) to that.
+1 on using ERRCODE_ARRAY_SUBSCRIPT_ERROR as it better conveys meaning.
cheers ./daniel
On 01/10/2020 14:21, Daniel Gustafsson wrote:
On 1 Oct 2020, at 12:54, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Most checks when converting between SQL and Python types use the PLy_elog() function, which uses the genericc ERRCODE_EXTERNAL_ROUTINE_EXCEPTION error code, but I think ERRCODE_ARRAY_SUBSCRIPT_ERROR is better.
On that note, wouldn't the dimension check errors in PLySequence_ToArray be
just as well off using normal ereport()'s? Only one of them seem to error out
in a way that could propagate an error from Python to postgres.
Yes, you are right. I was going to say that we are not very consistent
on when to use ereport() and when PLy_elog() in general, but looking
closer, we are. Outside the array-conversion functions, PLy_elog() is
only used when a Python C API call fails, and ereport() is used otherwise.
Patch attached.
- Heikki
Attachments:
0001-Tidy-up-error-reporting-when-converting-PL-Python-ar.patchtext/x-patch; charset=UTF-8; name=0001-Tidy-up-error-reporting-when-converting-PL-Python-ar.patchDownload
From 38a43bff396ec1f218ba709c780527c033189786 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 2 Oct 2020 14:36:56 +0300
Subject: [PATCH 1/1] Tidy up error reporting when converting PL/Python arrays.
Use PLy_elog() only when a call to a Python C API function failed, and
ereport() for other errors. Add an error code to the "wrong length of
inner sequence" ereport().
Reviewed-by: Daniel Gustafsson
Discussion: https://www.postgresql.org/message-id/B8B72889-D6D7-48FF-B782-D670A6CA4D37%40yesql.se
---
src/pl/plpython/plpy_typeio.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/pl/plpython/plpy_typeio.c b/src/pl/plpython/plpy_typeio.c
index 9e4e9035f7..b4aeb7fd59 100644
--- a/src/pl/plpython/plpy_typeio.c
+++ b/src/pl/plpython/plpy_typeio.c
@@ -1173,18 +1173,25 @@ PLySequence_ToArray(PLyObToDatum *arg, PyObject *plrv,
break;
if (ndim == MAXDIM)
- PLy_elog(ERROR, "number of array dimensions exceeds the maximum allowed (%d)", MAXDIM);
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("number of array dimensions exceeds the maximum allowed (%d)",
+ MAXDIM)));
dims[ndim] = PySequence_Length(pyptr);
if (dims[ndim] < 0)
PLy_elog(ERROR, "could not determine sequence length for function return value");
if (dims[ndim] > MaxAllocSize)
- PLy_elog(ERROR, "array size exceeds the maximum allowed");
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("array size exceeds the maximum allowed")));
len *= dims[ndim];
if (len > MaxAllocSize)
- PLy_elog(ERROR, "array size exceeds the maximum allowed");
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("array size exceeds the maximum allowed")));
if (dims[ndim] == 0)
{
@@ -1210,7 +1217,9 @@ PLySequence_ToArray(PLyObToDatum *arg, PyObject *plrv,
if (ndim == 0)
{
if (!PySequence_Check(plrv))
- PLy_elog(ERROR, "return value of function with array return type is not a Python sequence");
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("return value of function with array return type is not a Python sequence")));
ndim = 1;
len = dims[0] = PySequence_Length(plrv);
@@ -1256,7 +1265,8 @@ PLySequence_ToArray_recurse(PLyObToDatum *elm, PyObject *list,
if (PySequence_Length(list) != dims[dim])
ereport(ERROR,
- (errmsg("wrong length of inner sequence: has length %d, but %d was expected",
+ (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
+ errmsg("wrong length of inner sequence: has length %d, but %d was expected",
(int) PySequence_Length(list), dims[dim]),
(errdetail("To construct a multidimensional array, the inner sequences must all have the same length."))));
--
2.20.1
On 2 Oct 2020, at 13:44, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 01/10/2020 14:21, Daniel Gustafsson wrote:
On 1 Oct 2020, at 12:54, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Most checks when converting between SQL and Python types use the PLy_elog() function, which uses the genericc ERRCODE_EXTERNAL_ROUTINE_EXCEPTION error code, but I think ERRCODE_ARRAY_SUBSCRIPT_ERROR is better.On that note, wouldn't the dimension check errors in PLySequence_ToArray be
just as well off using normal ereport()'s? Only one of them seem to error out
in a way that could propagate an error from Python to postgres.Yes, you are right. I was going to say that we are not very consistent on when to use ereport() and when PLy_elog() in general, but looking closer, we are. Outside the array-conversion functions, PLy_elog() is only used when a Python C API call fails, and ereport() is used otherwise.
I did another scan as well and couldn't find any other offenders.
Patch attached.
LGTM.
cheers ./daniel
On 02/10/2020 15:06, Daniel Gustafsson wrote:
On 2 Oct 2020, at 13:44, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 01/10/2020 14:21, Daniel Gustafsson wrote:
On that note, wouldn't the dimension check errors in PLySequence_ToArray be
just as well off using normal ereport()'s? Only one of them seem to error out
in a way that could propagate an error from Python to postgres.Yes, you are right. I was going to say that we are not very
consistent on when to use ereport() and when PLy_elog() in general,
but looking closer, we are. Outside the array-conversion functions,
PLy_elog() is only used when a Python C API call fails, and
ereport() is used otherwise.I did another scan as well and couldn't find any other offenders.
Pushed. Thanks for checking!
- Heikki