Обсуждение: pl/perl example in the doc no longer works in 9.1
Hi, An example in the PG documentation gives an error: http://www.postgresql.org/docs/9.1/interactive/plperl-global.html CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$ $_SHARED{myquote} = sub { my $arg = shift; $arg =~s/(['\\])/\\$1/g; return "'$arg'"; }; $$ LANGUAGE plperl; SELECT myfuncs(); /* initializes the function */ ERROR: PL/Perl function must return reference to hash or array CONTEXT: PL/Perl function "myfuncs" Not sure if this is now an expected behaviour. Is it? Accordingly, I can open this in pgsql-bugs or put this issue in pgsql-docs.
On Oct 12, 2011, at 2:16 AM, Amit Khandekar wrote:
> CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
> $_SHARED{myquote} = sub {
> my $arg = shift;
> $arg =~ s/(['\\])/\\$1/g;
> return "'$arg'";
> };
> $$ LANGUAGE plperl;
>
> SELECT myfuncs(); /* initializes the function */
>
> ERROR: PL/Perl function must return reference to hash or array
> CONTEXT: PL/Perl function "myfuncs"
>
> Not sure if this is now an expected behaviour. Is it? Accordingly, I
> can open this in pgsql-bugs or put this issue in pgsql-docs.
Seems like there should be a bar return at the end of the function, otherwise it returns the last expression, which
happensto be a code reference. Not very useful in a function that should return VOID. New version:
CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$ $_SHARED{myquote} = sub { my $arg = shift; $arg =~
s/(['\\])/\\$1/g; return "'$arg'"; }; return;
$$ LANGUAGE plperl;
Best,
David
"David E. Wheeler" <david@kineticode.com> <CACoZds2D+0h-5euAxfpd9gQmiiW_MW9uv250Woz0=EGO0szScQ@mail.gmail.com> writes:
> On Oct 12, 2011, at 2:16 AM, Amit Khandekar wrote:
>> CREATE OR REPLACE FUNCTION myfuncs() RETURNS void AS $$
>> $_SHARED{myquote} = sub {
>> my $arg = shift;
>> $arg =~ s/(['\\])/\\$1/g;
>> return "'$arg'";
>> };
>> $$ LANGUAGE plperl;
>>
>> SELECT myfuncs(); /* initializes the function */
>>
>> ERROR: PL/Perl function must return reference to hash or array
>> CONTEXT: PL/Perl function "myfuncs"
>>
>> Not sure if this is now an expected behaviour. Is it? Accordingly, I
>> can open this in pgsql-bugs or put this issue in pgsql-docs.
> Seems like there should be a bar return at the end of the function, otherwise it returns the last expression, which
happensto be a code reference. Not very useful in a function that should return VOID. New version:
Well, the real question is why a function declared to return VOID cares
at all about what the last command in its body is. If this has changed
since previous versions then I think it's a bug and we should fix it,
not just change the example.
regards, tom lane
On Oct 12, 2011, at 9:15 AM, Tom Lane wrote: > Well, the real question is why a function declared to return VOID cares > at all about what the last command in its body is. If this has changed > since previous versions then I think it's a bug and we should fix it, > not just change the example. It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to add a bunch of bare return statements to existingfunctions. Best, David
"David E. Wheeler" <david@kineticode.com> writes:
> On Oct 12, 2011, at 9:15 AM, Tom Lane wrote:
>> Well, the real question is why a function declared to return VOID cares
>> at all about what the last command in its body is. If this has changed
>> since previous versions then I think it's a bug and we should fix it,
>> not just change the example.
> It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to add a bunch of bare return statements to
existingfunctions.
This appears to have gotten broken in commit
87bb2ade2ce646083f39d5ab3e3307490211ad04, which changed the function
return code to go through plperl_sv_to_datum, which is making
unwarranted assumptions ... but since it's utterly bereft of comments,
it's hard for a non Perl hacker to identify exactly what it should do
instead. The core of the problem seems to be that if SvROK(sv) then
the code assumes that it must be intended to convert that to an array or
composite, no matter whether the declared result type of the function is
compatible with such a thing. So I think this probably broke not only
VOID-result cases, but also cases where a Perl array or hash is supposed
to be returned as, say, text. It would be more appropriate to drive the
cases off the nature of the function result type, perhaps.
regards, tom lane
On Wed, Oct 12, 2011 at 15:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David E. Wheeler" <david@kineticode.com> writes:
>> On Oct 12, 2011, at 9:15 AM, Tom Lane wrote:
>>> Well, the real question is why a function declared to return VOID cares
>>> at all about what the last command in its body is. If this has changed
>>> since previous versions then I think it's a bug and we should fix it,
>>> not just change the example.
>
>> It has indeed changed, either in 9.0 or 9.1 (the latter, I think). I had to add a bunch of bare return statements to
existingfunctions.
[ For those that missed it, 9.0 is OK, it is indeed a bug in 9.1. ]
> This appears to have gotten broken in commit
> 87bb2ade2ce646083f39d5ab3e3307490211ad04, which changed the function
> return code to go through plperl_sv_to_datum, which is making
> unwarranted assumptions ... but since it's utterly bereft of comments,
> it's hard for a non Perl hacker to identify exactly what it should do
> instead.
Yeah, its a mess.
> The core of the problem seems to be that if SvROK(sv) then
> the code assumes that it must be intended to convert that to an array or
> composite, no matter whether the declared result type of the function is
> compatible with such a thing.
Hrm, well 9.0 and below did not get this "right" either:
create or replace function test_hash() returns text as $$ return
{'a'=>1}; $$ language plperl;
select test_array(); test_array
-----------------------ARRAY(0x7fd92384dcb8)
(1 row)
create or replace function test_hash() returns text as $$ return
{'a'=>1}; $$ language plperl;
select test_hash(); test_hash
----------------------HASH(0x7fd92387f848)
(1 row)
9.1 does this:
select test_array();test_array
------------\x01
(1 row)
select test_hash();
ERROR: type text is not composite
CONTEXT: PL/Perl function "test_hash"
> So I think this probably broke not only
> VOID-result cases, but also cases where a Perl array or hash is supposed
> to be returned as, say, text.
Given the output above (both pre 9.1 and post) it seems unless the
type is a set or composite we should throw an error. Maybe "PL/Perl
function returning type %s must not return a reference" ?
> It would be more appropriate to drive the
> cases off the nature of the function result type, perhaps.
Ill see if I can cook up something that's not too invasive.
[ I have a patch to fix the VOID issues. it gets rid of that horrid
has_retval variable/logic and makes it look much closer to 9.0's code.
Unfortunately its on my laptop at home as I was hacking on it before I
went to work... ]
On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker <badalex@gmail.com> wrote:
> On Wed, Oct 12, 2011 at 15:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The core of the problem seems to be that if SvROK(sv) then
>> the code assumes that it must be intended to convert that to an array or
>> composite, no matter whether the declared result type of the function is
>> compatible with such a thing.
>
> Hrm, well 9.0 and below did not get this "right" either:
> create or replace function test_hash() returns text as $$ return
> {'a'=>1}; $$ language plperl;
> select test_array();
> test_array
> -----------------------
> ARRAY(0x7fd92384dcb8)
> (1 row)
>
> create or replace function test_hash() returns text as $$ return
> {'a'=>1}; $$ language plperl;
> select test_hash();
> test_hash
> ----------------------
> HASH(0x7fd92387f848)
> (1 row)
>
> Given the output above (both pre 9.1 and post) it seems unless the
> type is a set or composite we should throw an error. Maybe "PL/Perl
> function returning type %s must not return a reference" ?
>
>> It would be more appropriate to drive the
>> cases off the nature of the function result type, perhaps.
>
> Ill see if I can cook up something that's not too invasive.
PFA my attempt at a fix.
This gets rid of of most of the if/else chain and the has_retval crap
in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
the lifting. It also now handles VOIDOID and checks that the request
result oid can be converted from the perl structure. For example if
you passed in a hashref with a result oid that was not an rowtype it
will error out with "PL/Perl cannot convert hash to non rowtype %s".
Arrays behave similarly.
One side effect is you can now return a composite literal where you
could not before. ( We already let you return array literals )
The comments might still be a bit sparse-- Im hoping the added errors
make things a bit more self explanatory.
A large portion of the diff is added regression tests, testing what
happens when you return various references.
Comments?
Вложения
On Oct 13, 2011, at 7:09 AM, Alex Hunsaker wrote:
> On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker <badalex@gmail.com> wrote:
>> On Wed, Oct 12, 2011 at 15:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>>> The core of the problem seems to be that if SvROK(sv) then
>>> the code assumes that it must be intended to convert that to an array or
>>> composite, no matter whether the declared result type of the function is
>>> compatible with such a thing.
>>
>> Hrm, well 9.0 and below did not get this "right" either:
>> create or replace function test_hash() returns text as $$ return
>> {'a'=>1}; $$ language plperl;
>> select test_array();
>> test_array
>> -----------------------
>> ARRAY(0x7fd92384dcb8)
>> (1 row)
>>
>> create or replace function test_hash() returns text as $$ return
>> {'a'=>1}; $$ language plperl;
>> select test_hash();
>> test_hash
>> ----------------------
>> HASH(0x7fd92387f848)
>> (1 row)
>>
>
>> Given the output above (both pre 9.1 and post) it seems unless the
>> type is a set or composite we should throw an error. Maybe "PL/Perl
>> function returning type %s must not return a reference" ?
>>
>>> It would be more appropriate to drive the
>>> cases off the nature of the function result type, perhaps.
>>
>> Ill see if I can cook up something that's not too invasive.
>
> PFA my attempt at a fix.
>
> This gets rid of of most of the if/else chain and the has_retval crap
> in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
> the lifting. It also now handles VOIDOID and checks that the request
> result oid can be converted from the perl structure. For example if
> you passed in a hashref with a result oid that was not an rowtype it
> will error out with "PL/Perl cannot convert hash to non rowtype %s".
> Arrays behave similarly.
>
> One side effect is you can now return a composite literal where you
> could not before. ( We already let you return array literals )
>
> The comments might still be a bit sparse-- Im hoping the added errors
> make things a bit more self explanatory.
>
> A large portion of the diff is added regression tests, testing what
> happens when you return various references.
>
> Comments?
Looks good at first sight and passes all regression tests for me.
--
Alexey Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.
Alex Hunsaker <badalex@gmail.com> writes:
> On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker <badalex@gmail.com> wrote:
>> On Wed, Oct 12, 2011 at 15:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> The core of the problem seems to be that if SvROK(sv) then
>>> the code assumes that it must be intended to convert that to an array or
>>> composite, no matter whether the declared result type of the function is
>>> compatible with such a thing.
> PFA my attempt at a fix.
> This gets rid of of most of the if/else chain and the has_retval crap
> in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
> the lifting. It also now handles VOIDOID and checks that the request
> result oid can be converted from the perl structure. For example if
> you passed in a hashref with a result oid that was not an rowtype it
> will error out with "PL/Perl cannot convert hash to non rowtype %s".
> Arrays behave similarly.
I'm working through this patch now. Does anyone object to having the
array-to-non-array-result-type and hash-to-non-rowtype-result-type cases
throw errors, rather than returning the rather useless ARRAY(...) and
HASH(...) strings as pre-9.1 did?
regards, tom lane
On Oct 13, 2011, at 11:02 AM, Tom Lane wrote: > I'm working through this patch now. Does anyone object to having the > array-to-non-array-result-type and hash-to-non-rowtype-result-type cases > throw errors, rather than returning the rather useless ARRAY(...) and > HASH(...) strings as pre-9.1 did? Certainly not in 9.2, no. Not sure about 9.1, though. Best, David
"David E. Wheeler" <david@kineticode.com> writes:
> On Oct 13, 2011, at 11:02 AM, Tom Lane wrote:
>> I'm working through this patch now. Does anyone object to having the
>> array-to-non-array-result-type and hash-to-non-rowtype-result-type cases
>> throw errors, rather than returning the rather useless ARRAY(...) and
>> HASH(...) strings as pre-9.1 did?
> Certainly not in 9.2, no. Not sure about 9.1, though.
Well, right now 9.1 is returning some rather random results. If we
don't change it, someone might claim that later releases ought to be
compatible with that ...
regards, tom lane
On Oct 13, 2011, at 11:25 AM, Tom Lane wrote: >> Certainly not in 9.2, no. Not sure about 9.1, though. > > Well, right now 9.1 is returning some rather random results. If we > don't change it, someone might claim that later releases ought to be > compatible with that ... Okay then, works for me. D
On Oct 13, 2011, at 9:02 PM, Tom Lane wrote: > Alex Hunsaker <badalex@gmail.com> writes: >> On Wed, Oct 12, 2011 at 15:33, Alex Hunsaker <badalex@gmail.com> wrote: >>> On Wed, Oct 12, 2011 at 15:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> The core of the problem seems to be that if SvROK(sv) then >>>> the code assumes that it must be intended to convert that to an array or >>>> composite, no matter whether the declared result type of the function is >>>> compatible with such a thing. > >> PFA my attempt at a fix. > >> This gets rid of of most of the if/else chain and the has_retval crap >> in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of >> the lifting. It also now handles VOIDOID and checks that the request >> result oid can be converted from the perl structure. For example if >> you passed in a hashref with a result oid that was not an rowtype it >> will error out with "PL/Perl cannot convert hash to non rowtype %s". >> Arrays behave similarly. > > I'm working through this patch now. Does anyone object to having the > array-to-non-array-result-type and hash-to-non-rowtype-result-type cases > throw errors, rather than returning the rather useless ARRAY(...) and > HASH(...) strings as pre-9.1 did? No objections here. -- Alexey Klyukin http://www.commandprompt.com The PostgreSQL Company – Command Prompt, Inc.
Alex Hunsaker <badalex@gmail.com> writes:
> This gets rid of of most of the if/else chain and the has_retval crap
> in plperl_handl_func(). Instead we let plperl_sv_to_datum() do most of
> the lifting. It also now handles VOIDOID and checks that the request
> result oid can be converted from the perl structure. For example if
> you passed in a hashref with a result oid that was not an rowtype it
> will error out with "PL/Perl cannot convert hash to non rowtype %s".
> Arrays behave similarly.
Applied with some further hacking of my own to clean up memory leaks
and grotty coding.
regards, tom lane
On Thu, Oct 13, 2011 at 16:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Applied with some further hacking of my own to clean up memory leaks > and grotty coding. Thanks! BTW after seeing it I agree passing in fcinfo (and the other fixes) to plperl_sv_to_datum() is better.