Re: better support of out parameters in plperl

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

Based on this analysis, and problems with differing regression results
on different platforms, this attached patch has been reverted.

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Uh, were are we in fixing/reviewing this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I think it has to wait to 8.3. It's a complete mess that was submitted
unheralded at the last moment. Pavel needs to get into the habit of
submitting ideas first, not just patches. And there must be proper
documentation and working regression tests.

cheers

andrew

Bruce Momjian wrote:

OK.

This has been saved for the 8.3 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I think it has to wait to 8.3. It's a complete mess that was submitted
unheralded at the last moment. Pavel needs to get into the habit of
submitting ideas first, not just patches. And there must be proper
documentation and working regression tests.

cheers

andrew

Bruce Momjian wrote:

Uh, were are we in fixing/reviewing this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Oh, let me add that this was first discussed on July 28:

http://archives.postgresql.org/pgsql-hackers/2006-07/msg01421.php

and a patch posted on July 30:

http://archives.postgresql.org/pgsql-hackers/2006-07/msg01559.php

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I think it has to wait to 8.3. It's a complete mess that was submitted
unheralded at the last moment. Pavel needs to get into the habit of
submitting ideas first, not just patches. And there must be proper
documentation and working regression tests.

cheers

andrew

Bruce Momjian wrote:

Uh, were are we in fixing/reviewing this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Like I said, at the last moment.

Bruce Momjian wrote:

Bruce Momjian <bruce@momjian.us> writes:

Uh, were are we in fixing/reviewing this?

It's dead for 8.2 --- Andrew's complaints are pretty serious at both
the conceptual and implementation levels, and there's been no sign of
discussion about how to fix them.

regards, tom lane

Would someone review this. It is in the patches_hold queue:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I think it has to wait to 8.3. It's a complete mess that was submitted
unheralded at the last moment. Pavel needs to get into the habit of
submitting ideas first, not just patches. And there must be proper
documentation and working regression tests.

cheers

andrew

Bruce Momjian wrote:

Uh, were are we in fixing/reviewing this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Has it been resubmitted with issues attended to? If it has, I missed it.
If not, why should someone else waste time on something so broken that
it produced a stringified perl hashref on output? It should never have
gone back in the queue IMNSHO.

cheers

andrew

Bruce Momjian wrote:

Andrew Dunstan wrote:

Has it been resubmitted with issues attended to? If it has, I missed it.
If not, why should someone else waste time on something so broken that
it produced a stringified perl hashref on output? It should never have
gone back in the queue IMNSHO.

Fine, removed. I don't understand Perl well enough to judge that.

---------------------------------------------------------------------------

cheers

andrew

Bruce Momjian wrote:

Would someone review this. It is in the patches_hold queue:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I think it has to wait to 8.3. It's a complete mess that was submitted
unheralded at the last moment. Pavel needs to get into the habit of
submitting ideas first, not just patches. And there must be proper
documentation and working regression tests.

cheers

andrew

Bruce Momjian wrote:

Uh, were are we in fixing/reviewing this?

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

This patch has been rejected based on comments just made by Andrew
Dunstan. If the author wants to revisit that, please reply and we can
discuss the issues.

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params. But names are optional, as I understand it. Arguably, we should be treating the returns positionally, and thus return an arrayref when there are OYT params, not a hashref, and ignore the names - after all, all perl function args are nameless, in fact, even if you use a naming convention to refer to them.

Fourth, I don't understand the change: "allow hash form output from procedures with one OUT argument." That seems very non-orthogonal, and I can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's no surprise at all.

The conversation regarding these features appears only to have started on July 28th, which was probably much too late given some of the issues. Unless we can solve these issues very fast I would be inclined to say this should be tabled for 8.3. I think this is a fairly good illustration of the danger of springing a feature, largely undiscussed, on the community just about freeze time.

cheers

andrew

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I'll look on it

From: Bruce Momjian <bruce@momjian.us>
To: Andrew Dunstan <andrew@dunslane.net>
CC: Pavel Stehule <pavel.stehule@hotmail.com>,
pgsql-patches@postgresql.org, david@fetter.org
Subject: Re: [PATCHES] better support of out parameters in plperl
Date: Thu, 8 Feb 2007 18:09:18 -0500 (EST)

This patch has been rejected based on comments just made by Andrew
Dunstan. If the author wants to revisit that, please reply and we can
discuss the issues.

---------------------------------------------------------------------------

Andrew Dunstan wrote:

I wrote:

Pavel Stehule wrote:

Hello,

I send two small patches. First does conversion from perl to
postgresql array in OUT parameters. Second patch allow hash form
output from procedures with one OUT argument.

I will try to review these in the next 2 weeks unless someone beats me
to it.

I have reviewed this lightly, as committed by Bruce, and have some
concerns. Unfortunately, the deathof my main workstation has cost me
much of the time I intended to use for a more thorough review, so there
may well be more issues than are outlined here.

First, it is completely undocumented.

Second, this comment is at best confusing:

/* if value is ref on array do to pg string array conversion */

Third, it appears to assume that we will have names for all OUT params.

But names are optional, as I understand it. Arguably, we should be treating
the returns positionally, and thus return an arrayref when there are OYT
params, not a hashref, and ignore the names - after all, all perl function
args are nameless, in fact, even if you use a naming convention to refer to
them.

Fourth, I don't understand the change: "allow hash form output from

procedures with one OUT argument." That seems very non-orthogonal, and I
can't see any good reason for it.

Lastly, if you look at the expected output as committed,it appears to

have been prepared without being actually examined, for example:

CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
return {a=>'ahoj'};
$$ LANGUAGE plperl;
SELECT '05' AS i,a FROM test05();
i | a
----+-----------------
05 | HASH(0x8558f9c)
(1 row)

what???

And now that I look I see every buildfarm box broken on PLCheck. That's

no surprise at all.

The conversation regarding these features appears only to have started

on July 28th, which was probably much too late given some of the issues.
Unless we can solve these issues very fast I would be inclined to say this
should be tabled for 8.3. I think this is a fairly good illustration of the
danger of springing a feature, largely undiscussed, on the community just
about freeze time.

cheers

andrew

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

_________________________________________________________________
Chcete sdilet sve obrazky a hudbu s prateli? http://messenger.msn.cz/