Re: [HACKERS] PL/Perl Returned Array

2011-08-17 Thread Andrew Dunstan



On 08/12/2011 09:17 PM, Alex Hunsaker wrote:

[empty arrays returned are not handled correctly]



Anyway, the attached patch fixes it for me. That is when we don't have
an array state, just return an empty array.  (Also adds some
additional comments)


Applied, thanks.

cheers

andrew


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


Re: [HACKERS] PL/Perl Returned Array

2011-08-17 Thread David E. Wheeler
On Aug 17, 2011, at 9:06 AM, Andrew Dunstan wrote:

 [empty arrays returned are not handled correctly]
 
 Anyway, the attached patch fixes it for me. That is when we don't have
 an array state, just return an empty array.  (Also adds some
 additional comments)
 
 Applied, thanks.

Awesome, thanks!

David

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


Re: [HACKERS] PL/Perl Returned Array

2011-08-17 Thread Alex Hunsaker
On Wed, Aug 17, 2011 at 10:06, Andrew Dunstan and...@dunslane.net wrote:


 On 08/12/2011 09:17 PM, Alex Hunsaker wrote:

 [empty arrays returned are not handled correctly]


 Anyway, the attached patch fixes it for me. That is when we don't have
 an array state, just return an empty array.  (Also adds some
 additional comments)

 Applied, thanks.

Thanks for picking this up.

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


Re: [HACKERS] PL/Perl Returned Array

2011-08-13 Thread Peter Eisentraut
On fre, 2011-08-12 at 20:09 -0700, Darren Duncan wrote:
 David E. Wheeler wrote:
  On Aug 12, 2011, at 6:17 PM, Alex Hunsaker wrote:
  Anyway, the attached patch fixes it for me. That is when we don't have
  an array state, just return an empty array.  (Also adds some
  additional comments)
  
  Fix confirmed, thank you!
  
  +1 to getting this committed before the next beta/RC.
 
 Policy question.  If this problem hadn't been detected until after 9.1 was 
 declared production, would it have been considered a bug to fix in 9.1.x or 
 would it have been delayed to 9.2 since that fix might be considered an 
 incompatibility?

Surely this would be a bug fix.


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


[HACKERS] PL/Perl Returned Array

2011-08-12 Thread David E. Wheeler
Hackers,

Given this script on 9.1beta3:

BEGIN;

CREATE EXTENSION plperl;

CREATE OR REPLACE FUNCTION wtf(
) RETURNS TEXT[] LANGUAGE plperl AS $$ return []; $$;

SELECT wtf() = '{}'::TEXT[];

ROLLBACK;

The output is:

BEGIN
CREATE EXTENSION
CREATE FUNCTION
 ?column? 
--
 f
(1 row)

ROLLBACK

Why is that? If I save the values to TEXT[] columns, they are still not the 
same. But if I pg_dump them and then load them up again, they *are* the same. 
The dump looks like this:

CREATE TABLE gtf (
have text[],
want text[]
);

COPY gtf (have, want) FROM stdin;
{}  {}
\.


Is PL/Perl doing something funky under the hood to array values it returns on 
9.1?

Thanks,

David


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


Re: [HACKERS] PL/Perl Returned Array

2011-08-12 Thread Alex Hunsaker
On Fri, Aug 12, 2011 at 18:00, David E. Wheeler da...@kineticode.com wrote:
 Hackers,

 Given this script on 9.1beta3:

    BEGIN;

    CREATE EXTENSION plperl;

    CREATE OR REPLACE FUNCTION wtf(
    ) RETURNS TEXT[] LANGUAGE plperl AS $$ return []; $$;

    SELECT wtf() = '{}'::TEXT[];

 Why is that? If I save the values to TEXT[] columns, they are still not the 
 same. But if I pg_dump them and then load them up again, they *are* the same. 
 The dump looks like this:

Eek.

 Is PL/Perl doing something funky under the hood to array values it returns on 
 9.1?

Yeah, its not handling empty arrays very well (its calling
accumArrayResult which increments the number of elements even when we
are a zero length array).

This was rewritten in 9.1 as part of the array overhaul. Previously
returned arrays were converted into a string with the perl sub
encode_array_literal(), potgres would then convert that string into
the appropriate data type. Not only was that a tad inefficient, but it
failed to handle composite types nicely. In 9.1 you can pass in arrays
with composite types and the like, we figured it would be awfully nice
if you could return them as well :-)

Anyway, the attached patch fixes it for me. That is when we don't have
an array state, just return an empty array.  (Also adds some
additional comments)

I thought about doing this right after we set dims[0] in
plperl_array_to_datum(), but that would fail to handle nested empty
multidim arrays like [[]]. As long as you can do that via
ARRAY[ARRAY[]]... I figure plperl should support it to.

Thanks for the report!
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***
*** 1078,1091  _array_to_datum(AV *av, int *ndims, int *dims, int cur_depth,
  	int			i = 0;
  	int			len = av_len(av) + 1;
  
- 	if (len == 0)
- 		astate = accumArrayResult(astate, (Datum) 0, true, atypid, NULL);
- 
  	for (i = 0; i  len; i++)
  	{
  		SV		  **svp = av_fetch(av, i, FALSE);
  		SV		   *sav = svp ? get_perl_array_ref(*svp) : NULL;
  
  		if (sav)
  		{
  			AV		   *nav = (AV *) SvRV(sav);
--- 1078,1092 
  	int			i = 0;
  	int			len = av_len(av) + 1;
  
  	for (i = 0; i  len; i++)
  	{
+ 		/* fetch the array element */
  		SV		  **svp = av_fetch(av, i, FALSE);
+ 
+ 		/* see if this element is an array, if so get that */
  		SV		   *sav = svp ? get_perl_array_ref(*svp) : NULL;
  
+ 		/* multi-dimensional array? */
  		if (sav)
  		{
  			AV		   *nav = (AV *) SvRV(sav);
***
*** 1149,1154  plperl_array_to_datum(SV *src, Oid typid)
--- 1150,1158 
  	astate = _array_to_datum((AV *) SvRV(src), ndims, dims, 1, astate, typid,
  			 atypid);
  
+ 	if (!astate)
+ 		return PointerGetDatum(construct_empty_array(atypid));
+ 
  	for (i = 0; i  ndims; i++)
  		lbs[i] = 1;
  

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


Re: [HACKERS] PL/Perl Returned Array

2011-08-12 Thread David E. Wheeler
On Aug 12, 2011, at 6:17 PM, Alex Hunsaker wrote:

 Anyway, the attached patch fixes it for me. That is when we don't have
 an array state, just return an empty array.  (Also adds some
 additional comments)

Fix confirmed, thank you!

+1 to getting this committed before the next beta/RC.

David


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


Re: [HACKERS] PL/Perl Returned Array

2011-08-12 Thread Darren Duncan

David E. Wheeler wrote:

On Aug 12, 2011, at 6:17 PM, Alex Hunsaker wrote:

Anyway, the attached patch fixes it for me. That is when we don't have
an array state, just return an empty array.  (Also adds some
additional comments)


Fix confirmed, thank you!

+1 to getting this committed before the next beta/RC.


Policy question.  If this problem hadn't been detected until after 9.1 was 
declared production, would it have been considered a bug to fix in 9.1.x or 
would it have been delayed to 9.2 since that fix might be considered an 
incompatibility?  If the latter, then I'm really glad it was found now. -- 
Darren Duncan


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