Re: [PATCHES] better support of out parameters in plperl
I'll look on it From: Bruce Momjian <[EMAIL PROTECTED]> To: Andrew Dunstan <[EMAIL PROTECTED]> CC: Pavel Stehule <[EMAIL PROTECTED]>, pgsql-patches@postgresql.org, [EMAIL PROTECTED] 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 <[EMAIL PROTECTED]> 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/ ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] better support of out parameters in plperl
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 <[EMAIL PROTECTED]> 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
Re: [PATCHES] better support of out parameters in plperl
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 [EMAIL PROTECTED] so that your >message can get through to the mailing list cleanly -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] better support of out parameters in plperl
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: 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 [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] better support of out parameters in plperl
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 <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] better support of out parameters in plperl
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] better support of out parameters in plperl
Like I said, at the last moment. Bruce Momjian wrote: 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] better support of out parameters in plperl
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 [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] better support of out parameters in plperl
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 [EMAIL PROTECTED] EnterpriseDBhttp://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
Re: [PATCHES] better support of out parameters in plperl
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 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] better support of out parameters in plperl
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 [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] better support of out parameters in plperl
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 [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: src/pl/plperl/plperl.c === RCS file: /cvsroot/pgsql/src/pl/plperl/plperl.c,v retrieving revision 1.115 retrieving revision 1.116 diff -c -r1.115 -r1.116 *** src/pl/plperl/plperl.c 12 Aug 2006 04:16:45 - 1.115 --- src/pl/plperl/plperl.c 13 Aug 2006 02:37:11 - 1.116 *** *** 1,7 /** * plperl.c - perl as a procedural language for PostgreSQL * ! * $PostgreSQL: pgsql/src/pl/plperl/plperl.c,v 1.115 2006/08/12 04:16:45 momjian Exp $ * **/ --- 1,7 /** * plperl.c - perl as a procedural language for PostgreSQL * ! * $PostgreSQL: pgsql/src/pl/plperl/plperl.c,v 1.116 2006/08/13 02:37:11 momjian Exp $ * **/ *** *** 52,57 --- 52,58 FmgrInfo result_in_func; /* I/O function and arg for result type */ Oid result_typioparam; int nargs; + int num_out_args; /* number of out arguments */ FmgrInfo arg_out_func[FUNC_MAX_ARGS]; bool arg_is_rowtype[FUNC_MAX_ARGS]; SV *reference; *** *** 115,120 --- 116,124 static void plperl_init_shared_libs(pTHX); static HV *plperl_spi_execute_fetch_result(SPITupleTable *, int, int); + static SV *plperl_convert_to_pg_array(SV *src); + static SV *plperl_transform_result(plperl_proc_desc *prodesc, SV *result); + /* * This routine is a crock, and so is everyplace that calls it. The problem * is that the cached form of plperl functions/queries is allocated permanently *** *** 404,410 (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("Perl hash contains nonexistent column \"%s\"", key))); ! if (SvOK(val) && SvTYPE(val) != SVt_NULL) values[attn - 1] = SvPV(val, PL_na); } hv_iterinit(perlhash); --- 408,419 (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("Perl hash contains nonexistent column \"%s\"", key))); ! ! /* if value is ref on array do to pg string array conversio
Re: [PATCHES] 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] better support of out parameters in plperl
Patch applied. Thanks. --- 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. > > Regards > Pavel Stehule > > _ > Citite se osamele? Poznejte nekoho vyjmecneho diky Match.com. > http://www.msn.cz/ [ Attachment, skipping... ] [ Attachment, skipping... ] > > ---(end of broadcast)--- > TIP 1: if posting/reading through Usenet, please send an appropriate >subscribe-nomail command to [EMAIL PROTECTED] so that your >message can get through to the mailing list cleanly -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] better support of out parameters in plperl
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. cheers andrew ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
[PATCHES] better support of out parameters in plperl
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. Regards Pavel Stehule _ Citite se osamele? Poznejte nekoho vyjmecneho diky Match.com. http://www.msn.cz/ *** ./plperl.c.orig 2006-07-29 21:07:09.0 +0200 --- ./plperl.c 2006-08-01 14:51:09.0 +0200 *** *** 117,122 --- 117,124 static void plperl_init_shared_libs(pTHX); static HV *plperl_spi_execute_fetch_result(SPITupleTable *, int, int); + static SV *plperl_convert_to_pg_array(SV *src); + /* * This routine is a crock, and so is everyplace that calls it. The problem * is that the cached form of plperl functions/queries is allocated permanently *** *** 412,418 (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("Perl hash contains nonexistent column \"%s\"", key))); ! if (SvOK(val) && SvTYPE(val) != SVt_NULL) values[attn - 1] = SvPV(val, PL_na); } hv_iterinit(perlhash); --- 414,425 (errcode(ERRCODE_UNDEFINED_COLUMN), errmsg("Perl hash contains nonexistent column \"%s\"", key))); ! ! /* if value is ref on array do to pg string array conversion */ ! if (SvTYPE(val) == SVt_RV && ! SvTYPE(SvRV(val)) == SVt_PVAV) ! values[attn - 1] = SvPV(plperl_convert_to_pg_array(val), PL_na); ! else if (SvOK(val) && SvTYPE(val) != SVt_NULL) values[attn - 1] = SvPV(val, PL_na); } hv_iterinit(perlhash); *** *** 1767,1773 if (SvOK(sv) && SvTYPE(sv) != SVt_NULL) { ! char *val = SvPV(sv, PL_na); ret = InputFunctionCall(&prodesc->result_in_func, val, prodesc->result_typioparam, -1); --- 1774,1789 if (SvOK(sv) && SvTYPE(sv) != SVt_NULL) { ! char *val; ! SV *array_ret; ! ! if (SvROK(sv) && SvTYPE(SvRV(sv)) == SVt_PVAV ) ! { ! array_ret = plperl_convert_to_pg_array(sv); ! sv = array_ret; ! } ! ! val = SvPV(sv, PL_na); ret = InputFunctionCall(&prodesc->result_in_func, val, prodesc->result_typioparam, -1); *** ./sql/plperl.sql.orig 2006-07-30 22:52:04.0 +0200 --- ./sql/plperl.sql 2006-08-01 15:02:53.0 +0200 *** *** 337,339 --- 337,374 $$ LANGUAGE plperl; SELECT * from perl_spi_prepared_set(1,2); + --- + --- Some OUT and OUT array tests + --- + + CREATE OR REPLACE FUNCTION test_out_params(OUT a varchar, OUT b varchar) AS $$ + return { a=> 'ahoj', b=>'svete'}; + $$ LANGUAGE plperl; + SELECT '01' AS i, * FROM test_out_params(); + + CREATE OR REPLACE FUNCTION test_out_params_array(OUT a varchar[], OUT b varchar[]) AS $$ + return { a=> ['ahoj'], b=>['svete']}; + $$ LANGUAGE plperl; + SELECT '02' AS i, * FROM test_out_params_array(); + + CREATE OR REPLACE FUNCTION test_out_params_set(OUT a varchar, out b varchar) RETURNS SETOF RECORD AS $$ + return_next { a=> 'ahoj', b=>'svete'}; + return_next { a=> 'ahoj', b=>'svete'}; + return_next { a=> 'ahoj', b=>'svete'}; + $$ LANGUAGE plperl; + SELECT '03' AS I,* FROM test_out_params_set(); + + CREATE OR REPLACE FUNCTION test_out_params_set_array(OUT a varchar[], out b varchar[]) RETURNS SETOF RECORD AS $$ + return_next { a=> ['ahoj'], b=>['velky','svete']}; + return_next { a=> ['ahoj'], b=>['velky','svete']}; + return_next { a=> ['ahoj'], b=>['velky','svete']}; + $$ LANGUAGE plperl; + SELECT '04' AS I,* FROM test_out_params_set_array(); + + + DROP FUNCTION test_out_params(); + DROP FUNCTION test_out_params_set(); + DROP FUNCTION test_out_params_array(); + DROP FUNCTION test_out_params_set_array(); + + *** ./plperl.c.orig 2006-08-01 15:20:16.0 +0200 --- ./plperl.c 2006-08-01 15:45:50.0 +0200 *** *** 52,57 --- 52,58 FmgrInfo result_in_func; /* I/O function and arg for result type */ Oid result_typioparam; int nargs; + int num_out_args; /* number of out arguments */ FmgrInfo arg_out_func[FUNC_MAX_ARGS]; bool arg_is_rowtype[FUNC_MAX_ARGS]; SV *reference; *** *** 118,123 --- 119,125 static HV *plperl_spi_execute_fetch_result(SPITupleTable *, int, int); static SV *plperl_convert_to_pg_array(SV *src); + static SV *plperl_transform_result(plperl_proc_desc *prodesc, SV *result); /* * This routine is a crock, and so is everyplace that calls it. The problem *** *** 698,709 HeapTuple tuple; Form_pg_proc proc; char functyptype; - int numargs; - Oid *argtypes; - char **argnames; - char *argmodes; bool istrigger = false; - int i; /* Get the new function's pg_proc entry */ tuple = SearchSysCache(PROCOID, --- 700,706 *** *** 731,748 format_type_be(proc->prorettype; } - /* Disallow pseudotypes in arguments (either IN o