Re: arrays as pl/perl input arguments [PATCH]

Hello,

Here's the patch that improves handling of arrays as pl/perl function input
arguments, converting postgres arrays of arbitrary dimensions into perl array
references. It includes regression tests and a documentation changes, and it
builds and runs successfully on my mac os x and linux boxes. To maintain
compatibility with existing pl/perl code a new variable,
plperl.convert_array_arguments (better name?), is introduced. Its default
value is false, when set to true it triggers the new behavior, i.e.

SET plperl.convert_array_arguments = true;
CREATE OR REPLACE FUNCTION test_array(text[]) RETURNS TEXT AS $$
my $arg = shift;
if (ref $arg eq 'ARRAY') {
return "array conversion is enabled";
} else {
return "array conversion is disabled";
}
$$ LANGUAGE plperl;

test=# select test_array('{1,2,3}');
test_array
-----------------------------
array conversion is enabled
(1 row)

You can find other, less trivial examples, by examining plperl_array
regression test.

The implementation detects whether the input argument of a perl function is an
array, and calls plperl_ref_from_pg_array if it is. The latter obtains a flat
array of Datums from deconstruct_array and, using information about array
dimensions, recursively creates perl array references in split_array. To pass
array information between recursive calls a new 'plperl_array_info' struct was
added. Arrays as members of composite types are also handled in
plperl_hash_from_tuple.

/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Jan 11, 2011, at 2:25 PM, Alexey Klyukin wrote:

> Hello,
>
> Here's the patch that improves handling of arrays as pl/perl function input
> arguments, converting postgres arrays of arbitrary dimensions into perl array
> references.

Awesome! I've wanted this for *years*.

> It includes regression tests and a documentation changes, and it
> builds and runs successfully on my mac os x and linux boxes. To maintain
> compatibility with existing pl/perl code a new variable,
> plperl.convert_array_arguments (better name?), is introduced. Its default
> value is false, when set to true it triggers the new behavior, i.e.

Have you considered instead passing an array-based object with is string overloading designed to return the pg array string format? That would make for nice, transparent compatibility without the need for a GUC.

Not my idea, BTW, but suggested by Tim Bunce.

Best,

David

On 01/11/2011 06:07 PM, David E. Wheeler wrote:
>
>> To maintain
>> compatibility with existing pl/perl code a new variable,
>> plperl.convert_array_arguments (better name?), is introduced. Its default
>> value is false, when set to true it triggers the new behavior, i.e.
> Have you considered instead passing an array-based object with is string overloading designed to return the pg array string format? That would make for nice, transparent compatibility without the need for a GUC.
>
> Not my idea, BTW, but suggested by Tim Bunce.
>
>

I think there's at least a danger of breaking legacy code doing that.
Say you have some code that does a ref test on the argument, for
example. The behavior would now be changed.

cheers

andrew

On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:

> I think there's at least a danger of breaking legacy code doing that. Say you have some code that does a ref test on the argument, for example. The behavior would now be changed.

I think that'd be pretty rare.

David

On 01/11/2011 07:17 PM, David E. Wheeler wrote:
> On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:
>
>> I think there's at least a danger of breaking legacy code doing that. Say you have some code that does a ref test on the argument, for example. The behavior would now be changed.
> I think that'd be pretty rare.
>
>

Possibly it would. But we usually try pretty hard to avoid that sort of
breakage.

cheers

andrew

On Tue, Jan 11, 2011 at 7:55 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On 01/11/2011 07:17 PM, David E. Wheeler wrote:
>> On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:
>>
>>> I think there's at least a danger of breaking legacy code doing that. Say
>>> you have some code that does a ref test on the argument, for example. The
>>> behavior would now be changed.
>>
>> I think that'd be pretty rare.
>
> Possibly it would. But we usually try pretty hard to avoid that sort of
> breakage.

By the same token, I'm not convinced it's a good idea for this
behavior to be off by default. Surely many people will altogether
fail to notice that it's an option? If we're going to have a
backward-compatibility GUC at all, ISTM that you ought to get the good
stuff unless you ask for the old way.

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

On 01/11/2011 09:06 PM, Robert Haas wrote:
> On Tue, Jan 11, 2011 at 7:55 PM, Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>> On 01/11/2011 07:17 PM, David E. Wheeler wrote:
>>> On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:
>>>
>>>> I think there's at least a danger of breaking legacy code doing that. Say
>>>> you have some code that does a ref test on the argument, for example. The
>>>> behavior would now be changed.
>>> I think that'd be pretty rare.
>> Possibly it would. But we usually try pretty hard to avoid that sort of
>> breakage.
> By the same token, I'm not convinced it's a good idea for this
> behavior to be off by default. Surely many people will altogether
> fail to notice that it's an option? If we're going to have a
> backward-compatibility GUC at all, ISTM that you ought to get the good
> stuff unless you ask for the old way.
>

Sure, that seems reasonable.

cheers

andrew

On Jan 12, 2011, at 1:07 AM, David E. Wheeler wrote:

> On Jan 11, 2011, at 2:25 PM, Alexey Klyukin wrote:
>
>> Hello,
>>
>> Here's the patch that improves handling of arrays as pl/perl function input
>> arguments, converting postgres arrays of arbitrary dimensions into perl array
>> references.
>
> Awesome! I've wanted this for *years*.
>
>> It includes regression tests and a documentation changes, and it
>> builds and runs successfully on my mac os x and linux boxes. To maintain
>> compatibility with existing pl/perl code a new variable,
>> plperl.convert_array_arguments (better name?), is introduced. Its default
>> value is false, when set to true it triggers the new behavior, i.e.
>
> Have you considered instead passing an array-based object with is string overloading designed to return the pg array string format? That would make for nice, transparent compatibility without the need for a GUC.

You mean packing both a string representation and a reference to a single SV * value?

I haven't considered that (lack of extensive perlgus-foo) although I think that's an interesting idea. One drawback would be that it would require both conversion to a string format and to a perl reference, performing unnecessary actions during every time arrays are passed to a pl/perl function. If there is a strong dislike of the proposed 'compatibility' GUC option - I think I can change the existing patch to incorporate array string representation into the reference-holding SV quite easily.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Jan 12, 2011, at 4:06 AM, Robert Haas wrote:

> On Tue, Jan 11, 2011 at 7:55 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> On 01/11/2011 07:17 PM, David E. Wheeler wrote:
>>> On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:
>>>
>>>> I think there's at least a danger of breaking legacy code doing that. Say
>>>> you have some code that does a ref test on the argument, for example. The
>>>> behavior would now be changed.
>>>
>>> I think that'd be pretty rare.
>>
>> Possibly it would. But we usually try pretty hard to avoid that sort of
>> breakage.
>
> By the same token, I'm not convinced it's a good idea for this
> behavior to be off by default. Surely many people will altogether
> fail to notice that it's an option? If we're going to have a
> backward-compatibility GUC at all, ISTM that you ought to get the good
> stuff unless you ask for the old way.

I think the number of people failing to notice the changes would be the same whenever we set the new or the old behavior by default. I decided to default to the the old behavior since it won't break the existing code as opposed to just hiding the good stuff, although it would slower the adoption of the new behavior.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:

> You mean packing both a string representation and a reference to a single SV * value?

Dunno, I'm not a guts guy.

> I haven't considered that (lack of extensive perlgus-foo) although I think that's an interesting idea. One drawback would be that it would require both conversion to a string format and to a perl reference, performing unnecessary actions during every time arrays are passed to a pl/perl function. If there is a strong dislike of the proposed 'compatibility' GUC option - I think I can change the existing patch to incorporate array string representation into the reference-holding SV quite easily.

Andrew's objections have merit. So perhaps just add this patch to the commit fest as is?

Best,

David

On Wed, Jan 12, 2011 at 06:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> On Jan 12, 2011, at 4:06 AM, Robert Haas wrote:
>> By the same token, I'm not convinced it's a good idea for this
>> behavior to be off by default.  Surely many people will altogether
>> fail to notice that it's an option?  If we're going to have a
>> backward-compatibility GUC at all, ISTM that you ought to get the good
>> stuff unless you ask for the old way.
>
> I think the number of people failing to notice the changes would be the same whenever we set the new or the old behavior by default. I decided to default to the the old behavior since it won't break the existing code as opposed to just hiding the good stuff, although it would slower the adoption of the new behavior.

Personally, I think the point of a compatibility GUC is that at some
point in the distant future we can get rid of it. If we default to
the old behavior thats going to be harder to do. +1 for defaulting to
the new behavior.

[ Id actually vote for _not_ having a compatibility option at all, we
change more major things than this IMHO every major release. (And even
then some major things in minor releases, for example the removal of
Safe.pm) ]

On Jan 12, 2011, at 11:22 AM, Alex Hunsaker wrote:

> Personally, I think the point of a compatibility GUC is that at some
> point in the distant future we can get rid of it. If we default to
> the old behavior thats going to be harder to do. +1 for defaulting to
> the new behavior.

+1

> [ Id actually vote for _not_ having a compatibility option at all, we
> change more major things than this IMHO every major release. (And even
> then some major things in minor releases, for example the removal of
> Safe.pm) ]

Yeah, but the removal of Safe.pm actually *improved* compatibility. This patch, without the GUC, would break it.

Best,

David

On Wed, Jan 12, 2011 at 12:22:55PM -0700, Alex Hunsaker wrote:
> On Wed, Jan 12, 2011 at 06:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> >
> > On Jan 12, 2011, at 4:06 AM, Robert Haas wrote:
> >> By the same token, I'm not convinced it's a good idea for this
> >> behavior to be off by default.  Surely many people will
> >> altogether fail to notice that it's an option?  If we're going to
> >> have a backward-compatibility GUC at all, ISTM that you ought to
> >> get the good stuff unless you ask for the old way.
> >
> > I think the number of people failing to notice the changes would
> > be the same whenever we set the new or the old behavior by
> > default. I decided to default to the the old behavior since it
> > won't break the existing code as opposed to just hiding the good
> > stuff, although it would slower the adoption of the new behavior.
>
> Personally, I think the point of a compatibility GUC is that at some
> point in the distant future we can get rid of it. If we default to
> the old behavior thats going to be harder to do. +1 for defaulting
> to the new behavior.
>
> [ Id actually vote for _not_ having a compatibility option at all,
> we change more major things than this IMHO every major release. (And
> even then some major things in minor releases, for example the
> removal of Safe.pm) ]

+1 for changing the behavior to something sane with loud, specific
warnings in the release notes about what will break and how.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Excerpts from Alex Hunsaker's message of mié ene 12 16:22:55 -0300 2011:

> [ Id actually vote for _not_ having a compatibility option at all, we
> change more major things than this IMHO every major release. (And even
> then some major things in minor releases, for example the removal of
> Safe.pm) ]

I think the main question here is: how loudly is existing code going to
break? If the breakage is silent, it's going to be very problematic.
If functions fail to run at all, then we can live without the
compatibility option.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jan 12, 2011, at 11:36 AM, Alvaro Herrera wrote:

>> [ Id actually vote for _not_ having a compatibility option at all, we
>> change more major things than this IMHO every major release. (And even
>> then some major things in minor releases, for example the removal of
>> Safe.pm) ]
>
> I think the main question here is: how loudly is existing code going to
> break? If the breakage is silent, it's going to be very problematic.
> If functions fail to run at all, then we can live without the
> compatibility option.

I suspect it'd be quiet, unfortunately, since there are a bazillion ad hoc implementations of a Perl SQL array parser, and many of them, I suspect, don't complain if the string doesn't look like an SQL array. They would just parse a string like "ARRAY(0x118ee2a0)" and return an empty array, or a NULL.

Best,

David

Excerpts from David E. Wheeler's message of mié ene 12 16:39:56 -0300 2011:
> On Jan 12, 2011, at 11:36 AM, Alvaro Herrera wrote:
>
> >> [ Id actually vote for _not_ having a compatibility option at all, we
> >> change more major things than this IMHO every major release. (And even
> >> then some major things in minor releases, for example the removal of
> >> Safe.pm) ]
> >
> > I think the main question here is: how loudly is existing code going to
> > break? If the breakage is silent, it's going to be very problematic.
> > If functions fail to run at all, then we can live without the
> > compatibility option.
>
> I suspect it'd be quiet, unfortunately, since there are a bazillion ad hoc implementations of a Perl SQL array parser, and many of them, I suspect, don't complain if the string doesn't look like an SQL array. They would just parse a string like "ARRAY(0x118ee2a0)" and return an empty array, or a NULL.

I kinda doubt that a function failing in that way would pass any
testing.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jan 12, 2011, at 11:51 AM, Alvaro Herrera wrote:

>> I suspect it'd be quiet, unfortunately, since there are a bazillion ad hoc implementations of a Perl SQL array parser, and many of them, I suspect, don't complain if the string doesn't look like an SQL array. They would just parse a string like "ARRAY(0x118ee2a0)" and return an empty array, or a NULL.
>
> I kinda doubt that a function failing in that way would pass any
> testing.

What is this “testing” thing of which you speak?

David

On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:

> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>
>> You mean packing both a string representation and a reference to a single SV * value?
>
> Dunno, I'm not a guts guy.

Well, neither me (I haven't used much of the guts api there).

>
>> I haven't considered that (lack of extensive perlgus-foo) although I think that's an interesting idea. One drawback would be that it would require both conversion to a string format and to a perl reference, performing unnecessary actions during every time arrays are passed to a pl/perl function. If there is a strong dislike of the proposed 'compatibility' GUC option - I think I can change the existing patch to incorporate array string representation into the reference-holding SV quite easily.
>
> Andrew's objections have merit. So perhaps just add this patch to the commit fest as is?

Already done :)

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Jan 12, 2011, at 9:36 PM, Alvaro Herrera wrote:

> Excerpts from Alex Hunsaker's message of mié ene 12 16:22:55 -0300 2011:
>
>> [ Id actually vote for _not_ having a compatibility option at all, we
>> change more major things than this IMHO every major release. (And even
>> then some major things in minor releases, for example the removal of
>> Safe.pm) ]
>
> I think the main question here is: how loudly is existing code going to
> break? If the breakage is silent, it's going to be very problematic.
> If functions fail to run at all, then we can live without the
> compatibility option.

Not really loud. Perl won't even complain when you try to interpret a
reference as a string.

Since almost everyone votes for making the new behavior a default option I'm
inclined to do that change, although I'm against throwing out the
compatibility option. There are many other reasons except for PL/Perl for
people to upgrade to 9.1, let's not force them to rewrite their Perl code
if they were not planning to do that.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

Excerpts from David E. Wheeler's message of mié ene 12 16:55:17 -0300 2011:
> On Jan 12, 2011, at 11:51 AM, Alvaro Herrera wrote:
>
> >> I suspect it'd be quiet, unfortunately, since there are a bazillion ad hoc implementations of a Perl SQL array parser, and many of them, I suspect, don't complain if the string doesn't look like an SQL array. They would just parse a string like "ARRAY(0x118ee2a0)" and return an empty array, or a NULL.
> >
> > I kinda doubt that a function failing in that way would pass any
> > testing.
>
> What is this “testing” thing of which you speak?

Ha ha ha :-(

I wonder if there's a way to overload the string representation of the
array so that it throws an error.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alexey Klyukin <alexk(at)commandprompt(dot)com> writes:
> Since almost everyone votes for making the new behavior a default option I'm
> inclined to do that change, although I'm against throwing out the
> compatibility option. There are many other reasons except for PL/Perl for
> people to upgrade to 9.1, let's not force them to rewrite their Perl code
> if they were not planning to do that.

IMO a GUC for this completely sucks, because if you do need to convert
your Perl functions, the only way to do it is to have a flag day wherein
they all change at once. And what about people writing Perl functions
that they'd like to give to other people?

If you have to have a flag, the only useful sort of flag is one that can
be attached to individual functions. Compare what we did for
plpgsql.variable_conflict in 9.0. I don't know how practical that will
be in plperl, though.

I thought the idea of overloading the string representation to look like
the old style was a cute solution. If we don't have anyone at hand who
knows how to do that, let's find someone who does. Not break our users'
code because we're too lazy to find out how to do it properly.

regards, tom lane

On 01/12/2011 04:22 PM, Tom Lane wrote:
> Alexey Klyukin<alexk(at)commandprompt(dot)com> writes:
>> Since almost everyone votes for making the new behavior a default option I'm
>> inclined to do that change, although I'm against throwing out the
>> compatibility option. There are many other reasons except for PL/Perl for
>> people to upgrade to 9.1, let's not force them to rewrite their Perl code
>> if they were not planning to do that.
> IMO a GUC for this completely sucks, because if you do need to convert
> your Perl functions, the only way to do it is to have a flag day wherein
> they all change at once. And what about people writing Perl functions
> that they'd like to give to other people?
>
> If you have to have a flag, the only useful sort of flag is one that can
> be attached to individual functions. Compare what we did for
> plpgsql.variable_conflict in 9.0. I don't know how practical that will
> be in plperl, though.

I don't see why it should be terribly difficult. We have the source code
and we have a couple of powerful regex engines. Determining it it has a
string in some position like

# pragma: plperl.arrays_as_strings

doesn't seem onerous. It's just a SMOC.

It's not too hard to imagine other things that might be useful for.

> I thought the idea of overloading the string representation to look like
> the old style was a cute solution. If we don't have anyone at hand who
> knows how to do that, let's find someone who does. Not break our users'
> code because we're too lazy to find out how to do it properly.
>
>

What I was casting a bit of doubt on upthread was whether or not this
would work without possibly breaking some code, in possibly silent or
obscure ways. If I'm wrong about that, then by all means let's use some
perl Magic (that's a technical term) to achieve this. IIRC Alex recently
posted some code that might be instructive about this.

cheers

andrew

On Wed, Jan 12, 2011 at 4:45 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> I thought the idea of overloading the string representation to look like
>> the old style was a cute solution.  If we don't have anyone at hand who
>> knows how to do that, let's find someone who does.  Not break our users'
>> code because we're too lazy to find out how to do it properly.
>
> What I was casting a bit of doubt on upthread was whether or not this would
> work without possibly breaking some code, in possibly silent or obscure
> ways. If I'm wrong about that, then by all means let's use some perl Magic
> (that's a technical term) to achieve this. IIRC Alex recently posted some
> code that might be instructive about this.

I agree with your gut feeling. I suspect the Perl magic solution will
make no one very happy.

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

On Jan 12, 2011, at 3:29 PM, Robert Haas wrote:

>> What I was casting a bit of doubt on upthread was whether or not this would
>> work without possibly breaking some code, in possibly silent or obscure
>> ways. If I'm wrong about that, then by all means let's use some perl Magic
>> (that's a technical term) to achieve this. IIRC Alex recently posted some
>> code that might be instructive about this.
>
> I agree with your gut feeling. I suspect the Perl magic solution will
> make no one very happy.

Could do both, I suppose…

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Jan 11, 2011, at 3:44 PM, Andrew Dunstan wrote:
>> I think there's at least a danger of breaking legacy code doing that. Say you have some code that does a ref test on the argument, for example. The behavior would now be changed.

> I think that'd be pretty rare.

I'm not seeing how an unsupported fear that there *might* be some
incompatibilities is a good argument for instead adopting an approach
that absolutely, positively, guaranteed *WILL* break everybody's code.

regards, tom lane

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>> You mean packing both a string representation and a reference to a single SV * value?

> Dunno, I'm not a guts guy.

> I haven't considered that (lack of extensive perlgus-foo) although I
> think that's an interesting idea. One drawback would be that it would
> require both conversion to a string format and to a perl reference,
> performing unnecessary actions during every time arrays are passed to a
> pl/perl function.

I had supposed that it would be possible to do the string conversion
lazily, ie, only if the string value was actually demanded.

regards, tom lane

On Wed, Jan 12, 2011 at 22:12, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>> You mean packing both a string representation and a reference to a single SV * value?
>
>> Dunno, I'm not a guts guy.
>
>> I haven't considered that (lack of extensive perlgus-foo) although I
>> think that's an interesting idea. One drawback would be that it would
>> require both conversion to a string format and to a perl reference,
>> performing unnecessary actions during every time arrays are passed to a
>> pl/perl function.
>
> I had supposed that it would be possible to do the string conversion
> lazily, ie, only if the string value was actually demanded.

Yep, In-fact if we wanted we could even die (or throw an exception in
other language speak :) ) when the string value is demanded.

On Wed, Jan 12, 2011 at 14:45, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> What I was casting a bit of doubt on upthread was whether or not this would
> work without possibly breaking some code, in possibly silent or obscure
> ways.

I can't see how it would break, unless we did it wrong...

> If I'm wrong about that, then by all means let's use some perl Magic
> (that's a technical term) to achieve this. IIRC Alex recently posted some
> code that might be instructive about this.

There might be a more gutsy way to do this so that 'ref' gives you
back what you expected (would that be 'ARRAY' or 'SCALAR' ?), but
there is a simple pure perl solution using overload:
package PLPerl::ArgArray;
use overload '""' => \&to_str;

sub new
{
# note we bless an arrayref here instead of the usual hashref so
# you can use this 'object' as a normal array
return bless([(at)_], ... );
}

sub to_str
{
my $self = shift;
# yeah this is not right not correct :P
# we could also die here with something like "You are trying to use
an Array as a string" or whatever
return join(',', map { '{'. $_ .'}' } @{$self});
}
---------

On Thu, Jan 13, 2011 at 12:06:33AM -0700, Alex Hunsaker wrote:
> > I had supposed that it would be possible to do the string conversion
> > lazily, ie, only if the string value was actually demanded.
>
> Yep, In-fact if we wanted we could even die (or throw an exception in
> other language speak :) ) when the string value is demanded.

I played with this a little and it is fairly easy to make a variable
such that $a is the string representation and $a[0] the first value of
the array. The problem is that you can't pass such a variable into a
subroutine.

I was thinking however, if the parameters if the function have names
you can use, then you can make it work. $_[0] would still go the old
way, but the named parameters could be the array.

====================== cut ======================
#!/usr/bin/perl -w
use strict;
no strict 'vars';
package MyClass;

sub TIESCALAR {
my $class = shift;
my $self = shift;
return bless $self, $class;
}

sub FETCH {
my $self = shift;
return join(",", @$self);
}

my @a=(1,2);

tie $a, "MyClass", \(at)a;

print "\$a='$a'\n";
print "\$a[0]='$a[0]'\n";

--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patriotism is when love of your own people comes first; nationalism,
> when hate for people other than your own comes first.
> - Charles de Gaulle

On Thu, Jan 13, 2011 at 01:06, Martijn van Oosterhout <kleptog(at)svana(dot)org> wrote:
> On Thu, Jan 13, 2011 at 12:06:33AM -0700, Alex Hunsaker wrote:
>> > I had supposed that it would be possible to do the string conversion
>> > lazily, ie, only if the string value was actually demanded.
>>
>> Yep, In-fact if we wanted we could even die (or throw an exception in
>> other language speak :) ) when the string value is demanded.
>
> I played with this a little and it is fairly easy to make a variable
> such that $a is the string representation and $a[0] the first value of
> the array. The problem is that you can't pass such a variable into a
> subroutine.

[ snip ]
> my @a=(1,2);
>
> tie $a, "MyClass", \(at)a;
>
> print "\$a='$a'\n";
> print "\$a[0]='$a[0]'\n";

Erm... the reason you can't seem to pass it to any subroutines is its
actually 2 variables: $a, @a.
When you print "$a\n"; you are using the tied variable that uses @a;
And when you print "$a[0]\n"; you are accessing the array directly.
I think you just had an unfortunate variable name, otherwise strict
would have complained appropriately. :)

On Thu, Jan 13, 2011 at 2:06 AM, Martijn van Oosterhout
<kleptog(at)svana(dot)org> wrote:
> I played with this a little and it is fairly easy to make a variable
> such that $a is the string representation and $a[0] the first value of
> the array. The problem is that you can't pass such a variable into a
> subroutine.

I played with this too:

#!/usr/bin/perl -w

use strict;

package Pg::ArrayArg;

use overload
'""' => \&as_s,
'@{}' => \&as_a;

sub new {
my $proto = shift;
my $class = ref $proto || $proto;

bless {
string => shift,
array => shift
}, $class;
}

sub as_s {
shift->{ 'string' };
}

sub as_a {
shift->{ 'array' };
}

package main;

my $aa = Pg::ArrayArg->new( '{1,2}', [ 1, 2 ] );

printf "ref = %s\n", ref $aa;
print "string = $aa\n";
printf "string = %s\n", $aa;
printf "array index = (%s, %s)\n", $aa->[ 0 ], $aa->[ 1 ];
printf "array_ref = %s\n", scalar @$aa;

print "regexp test = ";
if ($aa =~ /^{(.*)}$/) {
print "looks like array\n";
printf "join of split = %s\n", join ';', split /,/, $1;
} else {
print "doesn't look like array\n";
}

Suppose one of these compatibility objects is passed into legacy code
as $_[0]. The problem is that 'ref $_[0]' will return 'Pg::ArrayArg'
instead of what it used to, '' (empty string). Other than that, I
think it performs as people would expect.

You could even change 'as_s' to generate the string on the fly as
requested instead of generating both representations at instantiation.

Just my $0.02.

On Jan 13, 2011, at 4:15 PM, Stephen J. Butler wrote:

> Suppose one of these compatibility objects is passed into legacy code
> as $_[0]. The problem is that 'ref $_[0]' will return 'Pg::ArrayArg'
> instead of what it used to, '' (empty string). Other than that, I
> think it performs as people would expect.

Well, frankly, since up to this patch you *never* got an ARRAY reference argument, who would be calling `ref` on it anyway?

> You could even change 'as_s' to generate the string on the fly as
> requested instead of generating both representations at instantiation.

Yep.

Best,

David

On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:
>
>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>
>>> You mean packing both a string representation and a reference to a single SV * value?
>>
>> Dunno, I'm not a guts guy.
>
> Well, neither me (I haven't used much of the guts api there).

Find attached a proof of concept that modifies Alexey's patch to do
the above (using the overload example I and others posted).

Arrays have a reference of 'PostgreSQL::InServer::ARRAY'-- mainly to
be consistent with the other PL/Perl packages we have. It also lets
you match ref() =~ m/ARRAY$/ to make it a tad easier to figure out if
something is an array.

The other thing to note is I only applied this to the 'top' array.
That is when you have a multidimensional array, its children are
'real' arrays:

create or replace function takes_array(text[]) returns void as $$
my $array = shift;

elog(NOTICE, ref $array);
elog(NOTICE, ref $_) for (@$array);

$$ language plperl;

select takes_array('{{1}, {2, 3}}'::text[]);
NOTICE: PostgreSQL::InServer::ARRAY
CONTEXT: PL/Perl function "takes_array"
NOTICE: ARRAY
CONTEXT: PL/Perl function "takes_array"
NOTICE: ARRAY
CONTEXT: PL/Perl function "takes_array"

We could change that so _all_ arrays have a ref of
PostgreSQL::InServer::ARRAY. However I thought it would be nice to
easily use built-ins, this way you can just 'cast' away just the top
level without having to recurse to children (my @arr = @$array).

This also will create a string representation if and when you try to
use it as a string. It currently lacks row support in the stringify
case (and I left the regression test Alexey added for that failing) so
we would need to fix that up if we want to go down this road.

Thoughts? Should I polish this a bit more? Or do we like the GUC better?

On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>
>> On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:
>>
>>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>>
>>>> You mean packing both a string representation and a reference to a single SV * value?
>>>
>>> Dunno, I'm not a guts guy.
>>
>> Well, neither me (I haven't used much of the guts api there).
>
> Find attached a proof of concept that modifies Alexey's patch to do
> the above (using the overload example I and others posted).
[ ... ]
> Thoughts?  Should I polish this a bit more?  Or do we like the GUC better?

So its been over a week with no comments. ISTM there were more people
against adding yet another GUC. Barring objection ill finish the
missing parts of the POC patch I posted and submit that.

Hi,

On Jan 26, 2011, at 8:45 PM, Alex Hunsaker wrote:

> On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>>
>>> On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:
>>>
>>>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>>>
>>>>> You mean packing both a string representation and a reference to a single SV * value?
>>>>
>>>> Dunno, I'm not a guts guy.
>>>
>>> Well, neither me (I haven't used much of the guts api there).
>>
>> Find attached a proof of concept that modifies Alexey's patch to do
>> the above (using the overload example I and others posted).
> [ ... ]
>> Thoughts? Should I polish this a bit more? Or do we like the GUC better?
>
> So its been over a week with no comments. ISTM there were more people
> against adding yet another GUC. Barring objection ill finish the
> missing parts of the POC patch I posted and submit that.

I've played with that patch just today. I found a problem with it, when I tried to use the array in a string context the backend segfaulted with: "WARNING: Deep recursion on subroutine "main::encode_array_literal" at -e line 74" just before the segfault. I think the problem is in the regexp check in 'encode_array_literal' (it's obviously reversed comparing with the original one), but it still segfaults after I fixed that.

Other than that, the approach looks good to me, I'm for eliminating the GUC setting in favor of it.

/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Wed, Jan 26, 2011 at 12:44, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> Hi,
>
> On Jan 26, 2011, at 8:45 PM, Alex Hunsaker wrote:
>
>> On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>>> On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>>>
>>>> On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:
>>>>
>>>>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>>>>
>>>>>> You mean packing both a string representation and a reference to a single SV * value?
>>>>>
>>>>> Dunno, I'm not a guts guy.
>>>>
>>>> Well, neither me (I haven't used much of the guts api there).
>>>
>>> Find attached a proof of concept that modifies Alexey's patch to do
>>> the above (using the overload example I and others posted).
>> [ ... ]
>>> Thoughts?  Should I polish this a bit more?  Or do we like the GUC better?
>>
>> So its been over a week with no comments. ISTM there were more people
>> against adding yet another GUC.  Barring objection ill finish the
>> missing parts of the POC patch I posted and submit that.
>
> I've played with that patch just today. I found a problem with it, when I tried to use the array in a string context the backend segfaulted with: "WARNING:  Deep recursion on subroutine "main::encode_array_literal" at -e line 74" just before the segfault. I think the problem is in the regexp check in 'encode_array_literal' (it's obviously reversed comparing with the original one),

Yeah, I noticed that after I sent it out :(.

> but it still segfaults after I fixed that.

I seem to recall fixing this post email as well... Can you provide the
function that broke so I can double check? (Or was it part of the
regression test?)

Thanks for taking the time to play with it.

On Jan 26, 2011, at 10:08 PM, Alex Hunsaker wrote:

> On Wed, Jan 26, 2011 at 12:44, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>> Hi,
>>
>> On Jan 26, 2011, at 8:45 PM, Alex Hunsaker wrote:
>>
>>> On Sat, Jan 15, 2011 at 15:48, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>>>> On Wed, Jan 12, 2011 at 13:04, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>>>>
>>>>> On Jan 12, 2011, at 8:52 PM, David E. Wheeler wrote:
>>>>>
>>>>>> On Jan 12, 2011, at 5:14 AM, Alexey Klyukin wrote:
>>>>>>
>>>>>>> You mean packing both a string representation and a reference to a single SV * value?
>>>>>>
>>>>>> Dunno, I'm not a guts guy.
>>>>>
>>>>> Well, neither me (I haven't used much of the guts api there).
>>>>
>>>> Find attached a proof of concept that modifies Alexey's patch to do
>>>> the above (using the overload example I and others posted).
>>> [ ... ]
>>>> Thoughts? Should I polish this a bit more? Or do we like the GUC better?
>>>
>>> So its been over a week with no comments. ISTM there were more people
>>> against adding yet another GUC. Barring objection ill finish the
>>> missing parts of the POC patch I posted and submit that.
>>
>> I've played with that patch just today. I found a problem with it, when I tried to use the array in a string context the backend segfaulted with: "WARNING: Deep recursion on subroutine "main::encode_array_literal" at -e line 74" just before the segfault. I think the problem is in the regexp check in 'encode_array_literal' (it's obviously reversed comparing with the original one),
>
> Yeah, I noticed that after I sent it out :(.
>
>> but it still segfaults after I fixed that.
>
> I seem to recall fixing this post email as well... Can you provide the
> function that broke so I can double check? (Or was it part of the
> regression test?)

Sure, it's really simple (and the plperl_array regressions tests exposes this problem as well):

CREATE OR REPLACE FUNCTION test_array(INTEGER[]) RETURNS TEXT AS $$
my $array = shift;
print "$array"."\n";
$$ LANGUAGE plperl;

/A

I will look into this problem tomorrow unless you'll be lucky to fix it before that. Thank you for the review and your patch.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Wed, Jan 26, 2011 at 13:35, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> On Jan 26, 2011, at 10:08 PM, Alex Hunsaker wrote:
>>> (it's obviously reversed comparing with the original one), but it still segfaults after I fixed that.

Ahh yep, the reason reversing the check did not fix it is order of
operations. I had this fixed, but I had some unrelated changes in my
tree. So I manually git add(ed) the plperl files so I could use git
diff --cached to make the diff. Then I fixed this issue, but forgot
to git-add the changes :(. That explains why make installcheck worked
for me, but the diff I made was broken.

If you add some parens around ref it should work:
....
if ref($arg) !~ /ARRAY/;

btw the next version I plan on posting will check more explicitly:
if ref($arg) !~ /^(?:ARRAY|PostgreSQL::InServer::ARRAY)$/;

Find attached v3 of the patch. changes include:
- fix deep recursion due to accidental reversal of check in encode_array_literal
- add proper support for stringifying composite/row types. I did not
find a good way to quote these from the perl on the fly, so instead we
compute it the same way we used to and store the string inside the new
object along with the array :(.
- misc whitespace and code touchups

Hi,

On Jan 27, 2011, at 9:31 AM, Alex Hunsaker wrote:

> Find attached v3 of the patch. changes include:
> - fix deep recursion due to accidental reversal of check in encode_array_literal
> - add proper support for stringifying composite/row types. I did not
> find a good way to quote these from the perl on the fly, so instead we
> compute it the same way we used to and store the string inside the new
> object along with the array :(.
> - misc whitespace and code touchups
> <pg_to_perl_arrays_v3.patch.gz>

Nice improvement. It passes all the regression tests on my OS X system. I have only a minor suggestion, I think is_array is worth mentioning in the utility functions chapter of the pl/perl documentation, it would be also more clear to use it in regression tests as opposed to manually checking whether the ref is equal to 'PostgreSQL::InServer::ARRAY'.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Thu, Jan 27, 2011 at 03:38, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> Hi,
>
> On Jan 27, 2011, at 9:31 AM, Alex Hunsaker wrote:
>
>> Find attached v3 of the patch.  changes include:
>> - fix deep recursion due to accidental reversal of check in encode_array_literal
>> - add proper support for stringifying composite/row types.  I did not
>> find a good way to quote these from the perl on the fly, so instead we
>> compute it the same way we used to and store the string inside the new
>> object along with the array :(.
>> - misc whitespace and code touchups
>> <pg_to_perl_arrays_v3.patch.gz>
>
>
> Nice improvement. It passes all the regression tests on my OS X system. I have only a minor suggestion, I think is_array is worth mentioning in the utility functions chapter of the pl/perl documentation, it would be also more clear to use it in regression tests as opposed to manually checking whether the ref is equal to  'PostgreSQL::InServer::ARRAY'.

Wait a second... Just who is reviewing who's patch? :P

Both done in the attached. I also renamed is_array() to
is_array_ref() for clarity (hopefully).

On Jan 29, 2011, at 12:27 AM, Alex Hunsaker wrote:

> On Thu, Jan 27, 2011 at 03:38, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>> Hi,
>>
>> On Jan 27, 2011, at 9:31 AM, Alex Hunsaker wrote:
>>
>>> Find attached v3 of the patch. changes include:
>>> - fix deep recursion due to accidental reversal of check in encode_array_literal
>>> - add proper support for stringifying composite/row types. I did not
>>> find a good way to quote these from the perl on the fly, so instead we
>>> compute it the same way we used to and store the string inside the new
>>> object along with the array :(.
>>> - misc whitespace and code touchups
>>> <pg_to_perl_arrays_v3.patch.gz>
>>
>>
>> Nice improvement. It passes all the regression tests on my OS X system. I have only a minor suggestion, I think is_array is worth mentioning in the utility functions chapter of the pl/perl documentation, it would be also more clear to use it in regression tests as opposed to manually checking whether the ref is equal to 'PostgreSQL::InServer::ARRAY'.
>
> Wait a second... Just who is reviewing who's patch? :P
>

Oh, sorry, got confused along the way :)

> Both done in the attached. I also renamed is_array() to
> is_array_ref() for clarity (hopefully).
> <pg_to_perl_arrays_v4.patch.gz>

Thank you.

I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.

The v5 is attached.

On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:

> I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
> PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.

Looks good. Marked as "Ready for committer"

I'm sorry I'm late to this party. I haven't been keeping up with pgsql-hackers.

I'd kind'a hoped that this functionality could be tied into extending
PL/Perl to handle named arguments. That way the perl variables
corresponding to the named arguments could be given references without
breaking any code.

Some observations on the current code (based on a quick skim):

- I'd like to see the conversion function exposed as a builtin
$ref = decode_array_literal("{...}");

- Every existing plperl function that takes arrays is going to get
slower due to the overhead of parsing the string and allocating the
array and all its elements.

- Some of those functions may not use the array at all and some may
simply pass it on as an argument to another function.

- Making the conversion lazy would be a big help.

Tim.

Hi,

On Feb 2, 2011, at 7:16 PM, Tim Bunce wrote:

> I'm sorry I'm late to this party. I haven't been keeping up with pgsql-hackers.

Better late than never :)

>
> I'd kind'a hoped that this functionality could be tied into extending
> PL/Perl to handle named arguments. That way the perl variables
> corresponding to the named arguments could be given references without
> breaking any code.

Franky I don't see a direct connection between conversion of arrays into array
references and supporting named arguments. Could you, please, show some
examples of how you hoped the functionality could be extended?

>
> Some observations on the current code (based on a quick skim):
>
> - I'd like to see the conversion function exposed as a builtin
> $ref = decode_array_literal("{...}");

In principal, I think that's not hard to built with the functionality provided
by this patch. I see this as an XS function which takes the array string,
calls the array_in to convert it to the array datum, and, finally, calls
plperl_ref_from_pg_array (provided by the patch) to produce the resulting
array reference.

>
> - Every existing plperl function that takes arrays is going to get
> slower due to the overhead of parsing the string and allocating the
> array and all its elements.

Well, per my understanding of Alex changes, the string parsing is not invoked
unless requested by referencing the array in a string context. Normally, onle
plperl_ref_from_pg_array will be invoked every time the function is called
with an array argument, which would take little time to convert the PostgreSQL
internal array representation (not a sting) to the perl references, but that's
no different from what is already done with composite type arguments, which
are converted to perl hash references on every corresponding function call.

>
> - Some of those functions may not use the array at all and some may
> simply pass it on as an argument to another function.

I don't see how it would be good to optimize for the functions that are
declared to get the array but in fact do nothing with it. And considering the
case of passing an array through to another function, it's currently not
possible to call another pl/perl function from an existing one directly, and
when passing muti-dimensional arrays to a perl function one would need to
convert it to the array references anyway.

>
> - Making the conversion lazy would be a big help.

Converting it to string is already lazy, and, per the argumens above, I don't
see a real benefit of lazy conversion to the perl reference (as comparing to
the hurdles of implementing that).

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Feb 3, 2011, at 4:23 AM, Alexey Klyukin wrote:

>> - Making the conversion lazy would be a big help.
>
> Converting it to string is already lazy, and, per the argumens above, I don't
> see a real benefit of lazy conversion to the perl reference (as comparing to
> the hurdles of implementing that).

I agree that we should prefer an actual array as the implementation and lazily provide a string when needed.

Best,

David

On Thu, Feb 03, 2011 at 02:23:32PM +0200, Alexey Klyukin wrote:
> Hi,
>
> On Feb 2, 2011, at 7:16 PM, Tim Bunce wrote:
>
> > I'm sorry I'm late to this party. I haven't been keeping up with pgsql-hackers.
>
> Better late than never :)
>
> > I'd kind'a hoped that this functionality could be tied into extending
> > PL/Perl to handle named arguments. That way the perl variables
> > corresponding to the named arguments could be given references without
> > breaking any code.
>
> Franky I don't see a direct connection between conversion of arrays
> into array references and supporting named arguments.

There is no direct connection. I wasn't clear, "tied" was too strong a
word for it.

> Could you, please, show some examples of how you hoped the
> functionality could be extended?

I wasn't suggesting extending the functionality, just a way of adding
the functionality that wouldn't risk impacting existing code.

Imagine that PL/Perl could handle named arguments:

CREATE FUNCTION join_list( separator text, list array ) AS $$
return join( $separator, @$list );
$$ LANGUAGE plperl;

The $list variable, magically created by PL/Perl, could be the array
reference created by your code, without altering the contents of @_.

Existing code that does the traditional explicit unpacking of @_
wouldn't be affected. So there would be no need for the string overload
hack which, although I suggested it, I'm a little uncomfortable with.
(The problems with recursion and the need for is_array_ref with
hardwired class names are a little troubling. Not to mention the
extra overheads in accessing the array.)

On the ther hand, a string version of the array would still need to be
created for @_.

> > Some observations on the current code (based on a quick skim):
> >
> > - I'd like to see the conversion function exposed as a builtin
> > $ref = decode_array_literal("{...}");
>
> In principal, I think that's not hard to built with the functionality provided
> by this patch. I see this as an XS function which takes the array string,
> calls the array_in to convert it to the array datum, and, finally, calls
> plperl_ref_from_pg_array (provided by the patch) to produce the resulting
> array reference.

I think that would be useful.

> > - Every existing plperl function that takes arrays is going to get
> > slower due to the overhead of parsing the string and allocating the
> > array and all its elements.
>
> Well, per my understanding of Alex changes, the string parsing is not invoked
> unless requested by referencing the array in a string context. Normally, onle
> plperl_ref_from_pg_array will be invoked every time the function is called
> with an array argument, which would take little time to convert the PostgreSQL
> internal array representation (not a string) to the perl references, but that's
> no different from what is already done with composite type arguments, which
> are converted to perl hash references on every corresponding function call.

I'd missed that it was using the internal array representation (obvious
in hindsight) but there's still a significant cost in allocating the SVs
that won't be used by existing code. Though I agree it's of the same
order as for composite types.

> > - Some of those functions may not use the array at all and some may
> > simply pass it on as an argument to another function.
>
> I don't see how it would be good to optimize for the functions that are
> declared to get the array but in fact do nothing with it. And considering the
> case of passing an array through to another function, it's currently not
> possible to call another pl/perl function from an existing one directly, and
> when passing muti-dimensional arrays to a perl function one would need to
> convert it to the array references anyway.

I was thinking of calls to other pl/perl functions via sql.

> > - Making the conversion lazy would be a big help.
>
> Converting it to string is already lazy, and, per the argumens above, I don't
> see a real benefit of lazy conversion to the perl reference (as comparing to
> the hurdles of implementing that).

I (now) agree. Thanks.

Tim.

On 02/03/2011 01:01 PM, Tim Bunce wrote:
>
> Imagine that PL/Perl could handle named arguments:
>
> CREATE FUNCTION join_list( separator text, list array ) AS $$
> return join( $separator, @$list );
> $$ LANGUAGE plperl;
>
> The $list variable, magically created by PL/Perl, could be the array
> reference created by your code, without altering the contents of @_.

I think that's getting way too subtle, and it would probably violate the
POLA. If we implement named arguments I would expect $list to be the
same as $_[0]

>
>
>>> - Every existing plperl function that takes arrays is going to get
>>> slower due to the overhead of parsing the string and allocating the
>>> array and all its elements.
>> Well, per my understanding of Alex changes, the string parsing is not invoked
>> unless requested by referencing the array in a string context. Normally, onle
>> plperl_ref_from_pg_array will be invoked every time the function is called
>> with an array argument, which would take little time to convert the PostgreSQL
>> internal array representation (not a string) to the perl references, but that's
>> no different from what is already done with composite type arguments, which
>> are converted to perl hash references on every corresponding function call.
> I'd missed that it was using the internal array representation (obvious
> in hindsight) but there's still a significant cost in allocating the SVs
> that won't be used by existing code. Though I agree it's of the same
> order as for composite types.
>

Well, the question seems to be whether or not it's a reasonable price to
pay. On the whole I'm inclined to think it is, especially when it can be
avoided by updating your code, which will be a saving in fragility and
complexity as well.

cheers

andrew

On tor, 2011-02-03 at 18:01 +0000, Tim Bunce wrote:
> Imagine that PL/Perl could handle named arguments:
>
> CREATE FUNCTION join_list( separator text, list array ) AS $$
> return join( $separator, @$list );
> $$ LANGUAGE plperl;
>
> The $list variable, magically created by PL/Perl, could be the array
> reference created by your code, without altering the contents of @_.

I would find that pretty surprising, since Perl itself doesn't even
provide named arguments.

On Thu, Feb 3, 2011 at 05:23, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> Hi,
>
> On Feb 2, 2011, at 7:16 PM, Tim Bunce wrote:
>
[...]
>> Some observations on the current code (based on a quick skim):
>>
>> - I'd like to see the conversion function exposed as a builtin
>>    $ref = decode_array_literal("{...}");
>
> In principal, I think that's not hard to built with the functionality provided
> by this patch. I see this as an XS function which takes the array string,

*cough* array string and Oid for the datatype right? :P

> calls the array_in to convert it to the array datum, and, finally, calls
> plperl_ref_from_pg_array (provided by the patch) to produce the resulting
> array reference.

>> - Every existing plperl function that takes arrays is going to get
>>  slower due to the overhead of parsing the string and allocating the
>>  array and all its elements.
>
> Well, per my understanding of Alex changes, the string parsing is not invoked
> unless requested by referencing the array in a string context.

Sorry, there does seem to be some confusion here. The first version I
posted did lazy conversion to a string using encode_array_literal().
Unfortunately that function falls over with row/composite types. I
don't see a way to quote those without knowing the datatype, so in
interest of having it be 'correct' instead of fast, I made it always
decode to an array ref _and_ string in the next version :-(.

I see a couple of ways to fix this:
1) Using PTR2IV store the datum pointer in the array pseudo object.
This way when used as a string we have the original Datum and can call
the correct OutputFunction on demand. It would also let us call
plperl_ref_from_pg_array lazily. I have not actually tried it, but it
should work.

2) Add encode_composite_literal($hashref, $typeoid) and store the
type/Oid in the pseudo object. We could then call that lazily from the
perl.

3) Add the column position to the pseudo object and quote
appropriately. Simpler than #1 or #2 but not as robust.

4) Maybe there is a way of setting composite columns by column instead
of by position? If so we can encode that way. However everything on
http://www.postgresql.org/docs/current/interactive/rowtypes.html that
has to do with the 'literal' format seems to be positional.

5) Decided its worth the performance hit to always decode to both. (
Note it may not be as big as people think, in the case that you return
the array reference we have to convert it to a string anyway )

On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
> PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.
>
> The v5 is attached.

One thing I find odd is we allow for nested arrays, but not nested
composites. For example:
=> create type foo as (a int[]);
=> create type foofoo as (b foo);

=> create or replace function fa(foofoo[]) returns void as $$
my $foofoo_array = shift;
warn ref $foofoo_array->[0] || 'SCALAR';
warn ref $foofoo_array->[0]->{'b'} || 'SCALAR';
$$ language plperl;
=> select fa(ARRAY[ROW(ROW(ARRAY[1]))]::foofoo[]);
WARNING: HASH at line 3.
CONTEXT: PL/Perl function "fa"
WARNING: SCALAR at line 4.
CONTEXT: PL/Perl function "fa"

Why is foofoo.b a scalar but foofoo is a hash? This is not unique to
the patch, it how nested composites are handled in general. That is
if you passed in ROW(ROW()), the first ROW would be a hash while the
2nd row would be a scalar.

On the other end, the same is true when returning. If you try to
return a nested composite or array, the child better be a string as it
wont let you pass a hash:

=> create type foo as (a int[]);
=> create or replace function foo_in(foo) returns foo as $$ shift; $$
language plperl;
=> create or replace function foo_out() returns foo as $$ return
{'a'=>[1,2,3]}; $$ language plperl;

=> -- this works with the patch because we treat the reference as a string
=> select foo_in('("{1,2,3}")'::foo);
foo_in
-------------
("{1,2,3}")

=> select foo_out();
ERROR: array value must start with "{" or dimension information
CONTEXT: PL/Perl function "foo_out"
STATEMENT: SELECT foo_out();

-- also works as a straight string
=> create or replace function foo_out_str() returns foo as $$ return
{'a'=>'{1,2,3}'}; $$ language plperl;
=> select foo_out_str();
foo_out_str
-------------
("{1,2,3}")
(1 row)

Anyone object to fixing the above as part of this patch? That is
making plperl_(build_tuple_result, modify_tuple, return_next,
hash_from_tuple) handle array and hash (composite/row) types
consistently? And _that_ would be to recurse and turn them from/into
perl objects. Thoughts?

On 02/03/2011 08:29 PM, Alex Hunsaker wrote:
> On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin<alexk(at)commandprompt(dot)com> wrote:
>> I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
>> PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.
>>
>> The v5 is attached.
> One thing I find odd is we allow for nested arrays, but not nested
> composites.

> This is not unique to
> the patch, it how nested composites are handled in general. That is
> if you passed in ROW(ROW()), the first ROW would be a hash while the
> 2nd row would be a scalar.
>
> On the other end, the same is true when returning. If you try to
> return a nested composite or array, the child better be a string as it
> wont let you pass a hash

I think the reason this is so has to do with the history. Composite
support came to plperl about the same time (in the 8.0 cycle, IIRC) that
we were getting more support for nested composites in Postgres, and they
sorta passed like ships in the night.

> Anyone object to fixing the above as part of this patch? That is
> making plperl_(build_tuple_result, modify_tuple, return_next,
> hash_from_tuple) handle array and hash (composite/row) types
> consistently? And _that_ would be to recurse and turn them from/into
> perl objects. Thoughts?
>

I have no objection to making the whole thing work recursively, in
principle, but will it break legacy code?

cheers

andrew

On Fri, Feb 4, 2011 at 10:29, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> Anyone object to fixing the above as part of this patch? That is
>> making plperl_(build_tuple_result, modify_tuple, return_next,
>> hash_from_tuple) handle array and hash (composite/row) types
>> consistently? And _that_ would be to recurse and turn them from/into
>> perl objects. Thoughts?
>>
>
>
> I have no objection to making the whole thing work recursively, in
> principle, but will it break legacy code?

It will certainly change how nested composites are represented on the
'input side'. I would argue its a bug the way it is now and also
violates the POLA. I think we should also remain backwards compatible
on the output side so you could still return a string:

-- how things are currently, manually assigning a composite-literal
(would continue to work)
=> create or replace function trigger_func() returns trigger as $$
$_TD->{'new'}{'nested_composite'} = {'a'=>'(1,2)'}';
return 'MODIFY';

-- it would also work with perl nested structures (currently broken)
=> create or replace function trigger_func() returns trigger as $$
$_TD->{'new'}{'nested_composite'} = {'a'=>{'b'=>1, 'c'=>2}};
return 'MODIFY';
$$

Or perhaps you are saying we should do something similar with what we
now do with arrays? The pseudo array dance that is.

On Thu, Feb 3, 2011 at 18:29, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>> I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
>> PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.
>>
>> The v5 is attached.

> One thing I find odd is we allow for nested arrays, but not nested
> composites.  For example:
...
> On the other end, the same is true when returning. If you try to
> return a nested composite or array, the child better be a string as it
> wont let you pass a hash:

So here is a v6 that does the above. It does so by providing a generic
plperl_sv_to_datum() function and converting the various places to use
it. One cool thing is you can now use the composite types or arrays
passed in (or returned from another spi!) as arguments for
spi_exec_prepared(). Before you would have to convert the hashes to
strings. It also provides a real plperl_convert_to_pg_array (now
plperl_array_to_datum) that can handle composite and other random
datatypes. This also means we don't have to convert arrays to a string
representation first. (which removes part of the argument for #5 @
http://archives.postgresql.org/pgsql-hackers/2011-02/msg00197.php)

I have attached a stand alone version of the above so its easier to
look over. The diffstat is fairly small (ignoring added regression
tests)
1 files changed, 259 insertions(+), 165 deletions(-)

Comments?

On 02/03/2011 01:20 PM, Andrew Dunstan wrote:
>>>> - Every existing plperl function that takes arrays is going to get
>>>> slower due to the overhead of parsing the string and allocating the
>>>> array and all its elements.
>>> Well, per my understanding of Alex changes, the string parsing is
>>> not invoked
>>> unless requested by referencing the array in a string context.
>>> Normally, onle
>>> plperl_ref_from_pg_array will be invoked every time the function is
>>> called
>>> with an array argument, which would take little time to convert the
>>> PostgreSQL
>>> internal array representation (not a string) to the perl references,
>>> but that's
>>> no different from what is already done with composite type
>>> arguments, which
>>> are converted to perl hash references on every corresponding
>>> function call.
>> I'd missed that it was using the internal array representation (obvious
>> in hindsight) but there's still a significant cost in allocating the SVs
>> that won't be used by existing code. Though I agree it's of the same
>> order as for composite types.
>>
>
> Well, the question seems to be whether or not it's a reasonable price
> to pay. On the whole I'm inclined to think it is, especially when it
> can be avoided by updating your code, which will be a saving in
> fragility and complexity as well.
>

Tim,

do you till have concerns about this, or are you happy for us to move
ahead on it?

cheers

andrew

On Feb 6, 2011, at 9:43 AM, Alex Hunsaker wrote:

> On Thu, Feb 3, 2011 at 18:29, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
>> On Mon, Jan 31, 2011 at 01:34, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>> I've looked at the patch and added a test for arrays exceeding or equal maximum dimensions to check, whether the recursive function won't bring surprises there. I've also added check_stack_depth calls to both split_array and plperl_hash_from_tuple. Note that the regression fails currently due to the incorrect error reporting in
>>> PostgreSQL, per http://archives.postgresql.org/pgsql-hackers/2011-01/msg02888.php.
>>>
>>> The v5 is attached.
>
>> One thing I find odd is we allow for nested arrays, but not nested
>> composites. For example:
> ...
>> On the other end, the same is true when returning. If you try to
>> return a nested composite or array, the child better be a string as it
>> wont let you pass a hash:
>
> So here is a v6 that does the above. It does so by providing a generic
> plperl_sv_to_datum() function and converting the various places to use
> it. One cool thing is you can now use the composite types or arrays
> passed in (or returned from another spi!) as arguments for
> spi_exec_prepared(). Before you would have to convert the hashes to
> strings. It also provides a real plperl_convert_to_pg_array (now
> plperl_array_to_datum) that can handle composite and other random
> datatypes. This also means we don't have to convert arrays to a string
> representation first. (which removes part of the argument for #5 @
> http://archives.postgresql.org/pgsql-hackers/2011-02/msg00197.php)
>
> I have attached a stand alone version of the above so its easier to
> look over. The diffstat is fairly small (ignoring added regression
> tests)
> 1 files changed, 259 insertions(+), 165 deletions(-)
>
> Comments?

Thanks, looks great to me. It passes all the tests on my OS X system. I wonder
what's the purpose of the amagic_call in get_perl_array_ref, instead of
calling newRV_noinc on the target SV * ?

Also, in array_to_datum (line 1050), if get_perl_array_ref returns NULL for
the first element of the corresponding dimension, but non-NULL for the second
one - it would use uninitialized dims[cur_depth] value in comparison (which,
although, would probably lead to the desired comparison result).

/A
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Tue, Feb 8, 2011 at 08:18, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> On Feb 6, 2011, at 9:43 AM, Alex Hunsaker wrote:

>> So here is a v6
>> ....
>> Comments?
>
> Thanks, looks great to me. It passes all the tests on my OS X system. I wonder
> what's the purpose of the amagic_call in get_perl_array_ref, instead of
> calling newRV_noinc on the target SV * ?

Well, you can't AV *av = (AV *)SvRV(sv); And the SV * amagic_call
returns is already a reference, so the newRV_noinc() would be
redundant no? It occurs to me instead of doing the amagic_call we
could just call the to_array method directly using perl_call_pv().
That would look more normal and less magic-- thats probably a good
thing?

> Also, in array_to_datum (line 1050), if get_perl_array_ref returns NULL for
> the first element of the corresponding dimension, but non-NULL for the second
> one - it would use uninitialized dims[cur_depth] value in comparison (which,
> although, would probably lead to the desired comparison result).

Good catch, will fix. All of those checks should be outside of the while loop.

While Im at it Ill also rebase against HEAD (im sure there will be
some conflicts with that other plperl patch that just went in ;)).

On Tue, Feb 8, 2011 at 10:33, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Tue, Feb 8, 2011 at 08:18, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>> Thanks, looks great to me. It passes all the tests on my OS X system. I wonder
>> what's the purpose of the amagic_call in get_perl_array_ref, instead of
>> calling newRV_noinc on the target SV * ?
>
> Well, you can't AV *av = (AV *)SvRV(sv); And the SV * amagic_call
> returns is already a reference, so the newRV_noinc() would be
> redundant no? It occurs to me instead of doing the amagic_call we
> could just call the to_array method directly using perl_call_pv().
> That would look more normal and less magic-- thats probably a good
> thing?

Err, even simpler would be to just access the 'array' member directly
out of the hash object.

On Tue, Feb 8, 2011 at 10:33, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Tue, Feb 8, 2011 at 08:18, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>
>> On Feb 6, 2011, at 9:43 AM, Alex Hunsaker wrote:
>
>>> So here is a v6
>>> ....
>>> Comments?
>>
>> Thanks, looks great to me. It passes all the tests on my OS X system. I wonder
>> what's the purpose of the amagic_call in get_perl_array_ref, instead of
>> calling newRV_noinc on the target SV * ?
>

> Err, even simpler would be to just access the 'array' member directly
out of the hash object.

Done as the above, an added bonus is we no longer have to SvREF_dec()
so its even simpler.

>> Also, in array_to_datum (line 1050), if get_perl_array_ref returns NULL for
>> the first element of the corresponding dimension, but non-NULL for the second
>> one - it would use uninitialized dims[cur_depth] value in comparison (which,
>> although, would probably lead to the desired comparison result).
>
> Good catch, will fix. All of those checks should be outside of the while loop.

I clearly needed more caffeine in my system when i wrote that. Partly
due to the shadowed "av" variable. I've fixed it by initiating all of
the dims to 0. I also renamed the shadowed av var to "nav" for nested
av.

> While Im at it Ill also rebase against HEAD (im sure there will be
> some conflicts with that other plperl patch that just went in ;)).

So the merge while not exactly trivial was fairly simple. However it
would be great if you could give it another look over.

Find attached v7 changes include:
- rebased against HEAD
- fix potential use of uninitialized dims[cur_depth]
- took out accidental (broken) hack to try and support composite types
in ::encode_array_literal (added in v4 or something)
- make_array_ref() now uses plperl_hash_from_datum() for composite
types instead of its own hand rolled version
- get_perl_array_ref() now grabs the 'array' directly instead of
through the magic interface for simplicity
- moved added static declarations to the "bottom" instead of being
half on top and half on bottom

On Feb 9, 2011, at 3:44 AM, Alex Hunsaker wrote:

>
> So the merge while not exactly trivial was fairly simple. However it
> would be great if you could give it another look over.
>
> Find attached v7 changes include:
> - rebased against HEAD
> - fix potential use of uninitialized dims[cur_depth]
> - took out accidental (broken) hack to try and support composite types
> in ::encode_array_literal (added in v4 or something)
> - make_array_ref() now uses plperl_hash_from_datum() for composite
> types instead of its own hand rolled version
> - get_perl_array_ref() now grabs the 'array' directly instead of
> through the magic interface for simplicity
> - moved added static declarations to the "bottom" instead of being
> half on top and half on bottom
> <pg_to_perl_arrays_v7.patch.gz>

Thank you very much, the new patch applies cleanly and passes all tests on my
system. The new get_perl_array_ref seems to be much more clear to me, than the
prev. magic call.

What was actually broken in encode_array_literal support of composite types
(it converted perl hashes to the literal composite-type constants, expanding
nested arrays along the way) ? I think it would be a useful extension of the
existing encode_array_literal.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Tue, Feb 08, 2011 at 09:40:38AM -0500, Andrew Dunstan wrote:
> On 02/03/2011 01:20 PM, Andrew Dunstan wrote:
> >
> >Well, the question seems to be whether or not it's a reasonable
> >price to pay. On the whole I'm inclined to think it is, especially
> >when it can be avoided by updating your code, which will be a
> >saving in fragility and complexity as well.
>
> do you till have concerns about this, or are you happy for us to
> move ahead on it?

[I'm not really paying close enough attention for you to put much weight
on my opinions, but...]

I can't see any major issues so I'm happy for you to move ahead.

Thanks!

Tim.

On Wed, Feb 9, 2011 at 08:24, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> What was actually broken in encode_array_literal support of composite types
> (it converted perl hashes to the literal composite-type constants, expanding
> nested arrays along the way) ? I think it would be a useful extension of the
> existing encode_array_literal.

Yeah, It does not work because it did not take into account the order
of composite columns. It always put them alphabetically by column
name. To do it properly we would need to pass in a typid or a column
order or something. Ideally we could expose the new
plperl_array_to_datum() to plperl functions in some manner.

Here is a longer perhaps more concrete example:

Imagine you have a composite type with two 'columns':
=> create type foo as (z int, a int);
=> create or replace function foo_pl(foo[]) returns foo[] as $$
my $arg = shift;
$$ language plperl;
=> select foo_pl('{(1,2), (3,4)}');

In the above $arg looks something like (ignoring the
PostgreSQL::InServer::ARRAY object) [{'a'=>2, 'z'=>1}, {'a'=>4,
'z'=>3}]. When we call encode_arary_literal() we need to put it back
in to composite literal form which is basically (ignoring the array)
("column_z", "column_a"). However without type information we don't
know the order of the columns, as the composite is represented as a
hash we get kind of stuck. The hack I did sorted the hash keys
alphabetically, which worked for the regression tests as they happened
to have their composite columns sorted alphabetically. But would break
for this example putting $arg->[0]{a} into z and $arg->[0]{z} into a.

On Feb 9, 2011, at 9:28 PM, Alex Hunsaker wrote:

> On Wed, Feb 9, 2011 at 08:24, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>
>> What was actually broken in encode_array_literal support of composite types
>> (it converted perl hashes to the literal composite-type constants, expanding
>> nested arrays along the way) ? I think it would be a useful extension of the
>> existing encode_array_literal.
>
> Yeah, It does not work because it did not take into account the order
> of composite columns. It always put them alphabetically by column
> name. To do it properly we would need to pass in a typid or a column
> order or something. Ideally we could expose the new
> plperl_array_to_datum() to plperl functions in some manner.

Damn, right. Each perl hash corresponds to multiple composite types, different
by the order of the type elements. Passing the typid sounds like a fair
requirement (and if it's missing we could assume that the order of columns in
composites doesn't matter to the caller).

Let me try implementing that as an XS interface to plperl_array_to_datum.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On 02/10/2011 08:15 AM, Alexey Klyukin wrote:
> On Feb 9, 2011, at 9:28 PM, Alex Hunsaker wrote:
>
>> On Wed, Feb 9, 2011 at 08:24, Alexey Klyukin<alexk(at)commandprompt(dot)com> wrote:
>>> What was actually broken in encode_array_literal support of composite types
>>> (it converted perl hashes to the literal composite-type constants, expanding
>>> nested arrays along the way) ? I think it would be a useful extension of the
>>> existing encode_array_literal.
>> Yeah, It does not work because it did not take into account the order
>> of composite columns. It always put them alphabetically by column
>> name. To do it properly we would need to pass in a typid or a column
>> order or something. Ideally we could expose the new
>> plperl_array_to_datum() to plperl functions in some manner.
> Damn, right. Each perl hash corresponds to multiple composite types, different
> by the order of the type elements. Passing the typid sounds like a fair
> requirement (and if it's missing we could assume that the order of columns in
> composites doesn't matter to the caller).
>
> Let me try implementing that as an XS interface to plperl_array_to_datum.

Are you intending this as a completion of the current patch or as 9.2
work? If the former you need to send it in real fast.

cheers

andrew

On Feb 10, 2011, at 9:44 PM, Andrew Dunstan wrote:

>
>
> On 02/10/2011 08:15 AM, Alexey Klyukin wrote:
>> On Feb 9, 2011, at 9:28 PM, Alex Hunsaker wrote:
>>
>>> On Wed, Feb 9, 2011 at 08:24, Alexey Klyukin<alexk(at)commandprompt(dot)com> wrote:
>>>> What was actually broken in encode_array_literal support of composite types
>>>> (it converted perl hashes to the literal composite-type constants, expanding
>>>> nested arrays along the way) ? I think it would be a useful extension of the
>>>> existing encode_array_literal.
>>> Yeah, It does not work because it did not take into account the order
>>> of composite columns. It always put them alphabetically by column
>>> name. To do it properly we would need to pass in a typid or a column
>>> order or something. Ideally we could expose the new
>>> plperl_array_to_datum() to plperl functions in some manner.
>> Damn, right. Each perl hash corresponds to multiple composite types, different
>> by the order of the type elements. Passing the typid sounds like a fair
>> requirement (and if it's missing we could assume that the order of columns in
>> composites doesn't matter to the caller).
>>
>> Let me try implementing that as an XS interface to plperl_array_to_datum.
>
>
> Are you intending this as a completion of the current patch or as 9.2 work? If the former you need to send it in real fast.

I'd like to extend the current patch, going to post the update by tomorrow.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Feb 10, 2011, at 11:26 PM, Alexey Klyukin wrote:

> On Feb 10, 2011, at 9:44 PM, Andrew Dunstan wrote:
>
>> On 02/10/2011 08:15 AM, Alexey Klyukin wrote:
>>>
>>> Let me try implementing that as an XS interface to plperl_array_to_datum.
>>
>>
>> Are you intending this as a completion of the current patch or as 9.2 work? If the former you need to send it in real fast.
>
> I'd like to extend the current patch, going to post the update by tomorrow.

So, here is the v8. Instead of rewriting the encode_array_literal I've added
another function, encode_type_literal (could use a better name). basically a
user-level interface to plperl_sv_to_datum, which accepts the perl variable
and the type name as a text string and returns the string representation of
the input variable according to the output function of the argument type, e..g:

do $$ elog(NOTICE, encode_type_literal([[1],[2],[3]], 'int[]'));$$ language plperl;
NOTICE: {{1},{2},{3}}

I can easily convert the encode_array_literal to just call this function, but
not encode_array_constructor, since the latter needs to produce its own string
representation, different from the result of the type output function. One
option would be traversing through the SV *, duplicating the efforts of
plperl_sv_to_datum, but I actually wonder what do we need the
encode_array_constructor (and encode_array_literal) functions for ? I
guess they were useful to pass array references to SPI, but per my
understanding it's possible to use perl array references in SPI functions
directly now, so are there any other use cases for these functions, which
justify the time to spend on updating them to support arrays of composites
(and shouldn't we also provide encode_composite_literal and
encode_composite_constructor as well) ?

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Fri, Feb 11, 2011 at 17:17, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> So, here is the v8. Instead of rewriting the encode_array_literal I've added
> another function, encode_type_literal (could use a better name).
> ...
> I can easily convert the encode_array_literal to just call this function, but
> not encode_array_constructor,
> but I actually wonder what do we need the
> encode_array_constructor (and encode_array_literal) functions for ? I
> guess they were useful to pass array references to SPI,

I dunno, Ill have to go dig through the archives.

> but per my
> understanding it's possible to use perl array references in SPI functions
> directly now, so are there any other use cases for these functions, which
> justify the time to spend on updating them to support arrays of composites

Probably not, but I dunno if we can just drop them out from people using them...

> (and shouldn't we also provide encode_composite_literal and
> encode_composite_constructor as well) ?

Well we should not need encode_composite_literal,
encode_type_literal() should work for that. I don't see a reason for
the _constructor variant so id vote against it unless there is a use
case.

Anyway in playing with this patch a bit more I found another bug
return [[]]; would segfault.

So find attached a v9 that:
- fixes above segfault

- made plperl_sv_to_literal vastly simpler (no longer uses SPI and
calls regtypin directly)

- removed extraneous </para> added in v8 in plperl.sgml (my docbook
stuff is broken, so I can't build them, hopefully there are no other
problems)

- we now on the fly (when its requested) make the backwards compatible
string for arrays (it was a few lines now that we have
plperl_sv_to_literal)

- make plperl.o depend on plperl_helpers.h (should have been done in
the utf8 patch)

Anyway barring any more bugs, I think this patch is ready.

On Feb 12, 2011, at 9:53 AM, Alex Hunsaker wrote:

> On Fri, Feb 11, 2011 at 17:17, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
> Anyway in playing with this patch a bit more I found another bug
> return [[]]; would segfault.
>
> So find attached a v9 that:
> - fixes above segfault
>
> - made plperl_sv_to_literal vastly simpler (no longer uses SPI and
> calls regtypin directly)
>
> - removed extraneous </para> added in v8 in plperl.sgml (my docbook
> stuff is broken, so I can't build them, hopefully there are no other
> problems)
>
> - we now on the fly (when its requested) make the backwards compatible
> string for arrays (it was a few lines now that we have
> plperl_sv_to_literal)
>
> - make plperl.o depend on plperl_helpers.h (should have been done in
> the utf8 patch)
>
> Anyway barring any more bugs, I think this patch is ready.
> <pg_to_perl_arrays_v9.patch.gz>

Thank you for finding and fixing the bugs there. I think removing the para was a mistake.
I specifically added it to fix the problem I had when building the docs:

openjade:plperl.sgml:353:8:E: end tag for "PARA" omitted, but OMITTAG NO was specified
openjade:plperl.sgml:201:2: start tag was here

After I re-added the closing </para> in plperl.sgml:235 these errors disappeared, and the
resulting html looks fine too. v10 with just this single change is attached.

/A

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Feb 15, 2011, at 6:39 AM, Alexey Klyukin wrote:

> After I re-added the closing </para> in plperl.sgml:235 these errors disappeared, and the
> resulting html looks fine too. v10 with just this single change is attached.

So is this ready for committer?

Best,

David

On Feb 15, 2011, at 7:45 PM, David E. Wheeler wrote:

> On Feb 15, 2011, at 6:39 AM, Alexey Klyukin wrote:
>
>> After I re-added the closing </para> in plperl.sgml:235 these errors disappeared, and the
>> resulting html looks fine too. v10 with just this single change is attached.
>
> So is this ready for committer?

Yes.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Feb 15, 2011, at 9:49 AM, Alexey Klyukin wrote:

>>> After I re-added the closing </para> in plperl.sgml:235 these errors disappeared, and the
>>> resulting html looks fine too. v10 with just this single change is attached.
>>
>> So is this ready for committer?
>
> Yes.

Awesom, thanks Alexey & Alex!

David

On Sat, Feb 12, 2011 at 02:17:12AM +0200, Alexey Klyukin wrote:
>
> So, here is the v8. Instead of rewriting the encode_array_literal I've added
> another function, encode_type_literal (could use a better name).

Given that encode_array_literal() encodes an _array_ as a literal,
I'd assume encode_type_literal() encodes a _type_ as a literal.

I'd suggest encode_typed_literal() as a better name.

Tim [just passing though]

Excerpts from Tim Bunce's message of mié feb 16 14:08:11 -0300 2011:
> On Sat, Feb 12, 2011 at 02:17:12AM +0200, Alexey Klyukin wrote:
> >
> > So, here is the v8. Instead of rewriting the encode_array_literal I've added
> > another function, encode_type_literal (could use a better name).

> Given that encode_array_literal() encodes an _array_ as a literal,
> I'd assume encode_type_literal() encodes a _type_ as a literal.
>
> I'd suggest encode_typed_literal() as a better name.

FYI I'm looking at this patch (v10), and I'll incorporate Tim's suggestion.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

I cleaned up the patch a bit -- result is v11, attached. I'll give it
another look tomorrow and hopefully commit it.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 02/16/2011 05:54 PM, Alvaro Herrera wrote:
> I cleaned up the patch a bit -- result is v11, attached. I'll give it
> another look tomorrow and hopefully commit it.
>

Thanks for picking this up.

cheers

andrew

On Wed, Feb 16, 2011 at 12:21, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Tim Bunce's message of mié feb 16 14:08:11 -0300 2011:
>> I'd suggest encode_typed_literal() as a better name.
>
> FYI I'm looking at this patch (v10), and I'll incorporate Tim's suggestion.

Looks good to me.

Excerpts from Alex Hunsaker's message of sáb feb 12 04:53:14 -0300 2011:

> - make plperl.o depend on plperl_helpers.h (should have been done in
> the utf8 patch)

Incidentally, I think this bit was lost, no?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Feb 17, 2011 at 16:18, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Alex Hunsaker's message of sáb feb 12 04:53:14 -0300 2011:
>
>> - make plperl.o depend on plperl_helpers.h (should have been done in
>> the utf8 patch)
>
> Incidentally, I think this bit was lost, no?

It was, yes.

Excerpts from Alvaro Herrera's message of mié feb 16 19:54:07 -0300 2011:
>
> I cleaned up the patch a bit -- result is v11, attached. I'll give it
> another look tomorrow and hopefully commit it.

Applied. Thanks.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support