Bad behavior from plpython 'return []'

Started by Jim Nasbyalmost 10 years ago4 messageshackers
Jump to latest
#1Jim Nasby
Jim.Nasby@BlueTreble.com

CREATE FUNCTION pg_temp.bad() RETURNS text[] LANGUAGE plpythonu AS
$$return []$$;
SELECT pg_temp.bad();
bad
-----
{}
(1 row)

SELECT pg_temp.bad() = '{}'::text[];
?column?
----------
f
(1 row)

Erm?? Turns out this is because

SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
array_dims | array_dims
------------+------------
[1:0] |
(1 row)

and array_eq does this right off the bat:

/* fast path if the arrays do not have the same dimensionality */
if (ndims1 != ndims2 ||
memcmp(dims1, dims2, ndims1 * sizeof(int)) != 0 ||
memcmp(lbs1, lbs2, ndims1 * sizeof(int)) != 0)
result = false;

plpython is calling construct_md_array() with ndims set to 1, *lbs=1 and
(I'm pretty sure) *dims=0. array_in throws that combination out as
bogus; I think that construct_md_array should at least assert() that as
well. It's only used in a few places outside of arrayfuncs.c, but I find
it rather disturbing that an included PL has been broken in this fashion
for quite some time (PLySequence_ToArray() is the same in 9.0). There's
at least one plpython unit test that would have thrown an assert.

plperl appears to be immune from this because it calls
accumArrayResult() inside a loop that shouldn't execute for a 0 length
array. Would that be the preferred method of building arrays in
plpython? ISTM that'd be wasteful since it would incur a useless copy
for everything that's varlena (AFAICT plperl already suffers from this).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Robert Haas
robertmhaas@gmail.com
In reply to: Jim Nasby (#1)
Re: Bad behavior from plpython 'return []'

On Thu, Jun 30, 2016 at 9:25 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

CREATE FUNCTION pg_temp.bad() RETURNS text[] LANGUAGE plpythonu AS $$return
[]$$;
SELECT pg_temp.bad();
bad
-----
{}
(1 row)

SELECT pg_temp.bad() = '{}'::text[];
?column?
----------
f
(1 row)

Erm?? Turns out this is because

SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
array_dims | array_dims
------------+------------
[1:0] |
(1 row)

Yeah, that's a bug.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#2)
Re: Bad behavior from plpython 'return []'

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Jun 30, 2016 at 9:25 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
array_dims | array_dims
------------+------------
[1:0] |
(1 row)

Yeah, that's a bug.

It looks like this is because PLySequence_ToArray neglects to special-case
zero-element arrays. We could fix it there, but this is not the first
such bug. I wonder if we should change construct_md_array to force
zero-element arrays to be converted to empty arrays, rather than assuming
callers will have short-circuited the case earlier. Something like

/* fast track for empty array */
if (ndims == 0)
return construct_empty_array(elmtype);

nelems = ArrayGetNItems(ndims, dims);

+	/* if caller tries to specify zero-length array, make it empty */
+	if (nelems <= 0)
+		return construct_empty_array(elmtype);
+
	/* compute required space */
	nbytes = 0;
	hasnulls = false;

But that might introduce new problems too, if any callers expect the
array dimensions to be exactly what they asked for.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Tom Lane (#3)
Re: Bad behavior from plpython 'return []'

On 7/1/16 2:52 PM, Tom Lane wrote:

+	/* if caller tries to specify zero-length array, make it empty */
+	if (nelems <= 0)
+		return construct_empty_array(elmtype);
+
/* compute required space */
nbytes = 0;
hasnulls = false;

But that might introduce new problems too, if any callers expect the
array dimensions to be exactly what they asked for.

You mean ndims? What if instead of an empty array it returned an array
where *dims was just all zeros (and correctly set *lbs)? array_eq would
still need to account for that, but I think we don't have a choice about
that unless we expressly forbid arrays where any of the elements of
*dims were 0 (which I suspect we should probably do anyway... I don't
see how you can do anything with a 2x0x3 array...)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers