Re: [REVIEW] Patch for cursor calling with named parameters

Initial Review for patch:

http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php

Submission review

The patch is in context diff format and applies cleanly to the git master. The patch includes an update to regression tests. The regression tests pass. The patch does not include updates to the documentation reflecting the new functionality.

Usability review

The patch adds a means of specifying named cursor parameter arguments in pg/plsql.

The syntax is straight forward:

cur1 CURSOR (param1 int, param2 int, param3 int) for select …;
…
open cur1(param3 := 4, param2 := 1, param1 := 5);

The old syntax continues to work:

cur1 CURSOR (param1 int, param2 int, param3 int) for select …;
…
open cur1(5, 1, 3);

• Does the patch actually implement that?

Yes, the feature works as advertised.

• Do we want that?

I very rarely use pg/plsql, so I won't speak to its utility. However there has been some discussion about the idea:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg01440.php

• Do we already have it?

Not AFAIK

• Does it follow SQL spec, or the community-agreed behavior?

There's some discussion about the syntax ( := or => ), I'm not sure there's a consensus yet.

• Does it include pg_dump support (if applicable)?

Yes -- pgplsql functions using named parameters are correctly dumped.

• Are there dangers?

Potentially. See below.

• Have all the bases been covered?

I don't think so. The new feature accepts opening a cursor with some parameter names not specified:

open cur1(param3 := 4, 1, param1 := 5);

It seems that if a parameter is not named, its position is used to bind to a variable. For example, the following fail:

psql:plsqltest.sql:26: ERROR: cursor "cur1" argument 2 "param2" provided multiple times
LINE 10: open cur1(param3 := 4, 1, param2 := 5);

and

psql:plsqltest.sql:26: ERROR: cursor "cur1" argument 2 "param2" provided multiple times
LINE 10: open cur1(param2 := 4, 2, param1 := 5);

I think that postgres ought to enforce some consistency here. Use one way or the other, never both.

I can also produce some unhelpful errors when I give bad syntax. For example:

psql:plsqltest.sql:28: ERROR: cursor "cur1" argument 1 "param1" provided multiple times
LINE 11: open cur1( param3 : = 4, 2, param1 := 5);
(notice the space between the : and =)

--

psql:plsqltest.sql:28: ERROR: cursor "cur1" argument 1 "param1" provided multiple times
LINE 11: open cur1( param3 => 4, 2, param1 := 5);
(Wrong assignment operator)

--

psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" provided multiple times
LINE 10: open cur1( 1 , param3 := 2, param2 = 3 );
(Wrong assignment operator)

--

psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" provided multiple times
LINE 10: ... open cur1( param3 = param3 , param3 := 2, param2 = 3 );

--

open cur1( param3 := param3 , param2 = 3, param1 := 1 );

psql:plsqltest.sql:29: ERROR: column "param2" does not exist
LINE 2: ,param2 = 3
^
QUERY: SELECT 1
,param2 = 3
,param3;
CONTEXT: PL/pgSQL function "named_para_test" line 7 at OPEN

I haven't been able to make something syntactically incorrect that doesn't produce an error, even if the error isn't spot on. I haven't been able to make it crash, and when the syntax is correct, it has always produced correct results.

Performance review

I've done some limited performance testing and I can't really see a difference between the unpatched and patched master.

• Does it follow the project coding guidelines?

I believe so, but someone more familiar with them will probably spot violations better than me =)

• Are there portability issues?

I wouldn't know -- I don't have any experience with C portability.

• Will it work on Windows/BSD etc?

Tested under OS X, so BSD is presumably okay. No idea about other unixes nor windows.

• Are the comments sufficient and accurate?

I'm happy enough with them.

• Does it do what it says, correctly?

Yes, excepting my comments above.

• Does it produce compiler warnings?

Yes:

In file included from gram.y:12962:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:16243: warning: unused variable ‘yyg’

But this was not added by this patch -- it's also in the unpatched master.

• Can you make it crash?

No.

--Royce

Royce Ausburn <royce(dot)ml(at)inomial(dot)com> writes:
> Initial Review for patch:
> http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php
> The patch adds a means of specifying named cursor parameter arguments in pg/plsql.

> Do we want that?

> I very rarely use pg/plsql, so I won't speak to its utility. However there has been some discussion about the idea:
> http://archives.postgresql.org/pgsql-hackers/2010-09/msg01440.php

I still think what I said in that message, which is that it's premature
to add this syntax to plpgsql cursors when we have thoughts of changing
it. There is not any groundswell of demand from the field for named
parameters to cursors, so I think we can just leave this in abeyance
until the function case has settled.

regards, tom lane

On Thu, Oct 6, 2011 at 12:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Royce Ausburn <royce(dot)ml(at)inomial(dot)com> writes:
>> Initial Review for patch:
>> http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php
>> The patch adds a means of specifying named  cursor parameter arguments in pg/plsql.
>
>>       • Do we want that?
>
>> I very rarely use pg/plsql, so I won't speak to its utility.  However there has been some discussion about the idea:
>> http://archives.postgresql.org/pgsql-hackers/2010-09/msg01440.php
>
> I still think what I said in that message, which is that it's premature
> to add this syntax to plpgsql cursors when we have thoughts of changing
> it.  There is not any groundswell of demand from the field for named
> parameters to cursors, so I think we can just leave this in abeyance
> until the function case has settled.

+1. However, if that's the route we're traveling down, I think we had
better go ahead and remove the one remaining => operator from hstore
in 9.2:

CREATE OPERATOR => (
LEFTARG = text,
RIGHTARG = text,
PROCEDURE = hstore
);

We've been warning that this operator name was deprecated since 9.0,
so it's probably about time to take the next step, if we want to have
a chance of getting this sorted out in finite time.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> +1. However, if that's the route we're traveling down, I think we had
> better go ahead and remove the one remaining => operator from hstore
> in 9.2:

Fair enough.

regards, tom lane

On Oct 6, 2011, at 9:46 AM, Tom Lane wrote:

>> +1. However, if that's the route we're traveling down, I think we had
>> better go ahead and remove the one remaining => operator from hstore
>> in 9.2:
>
> Fair enough.

Would it then be added as an alias for := for named function parameters? Or would that come still later?

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Oct 6, 2011, at 9:46 AM, Tom Lane wrote:
>>> +1. However, if that's the route we're traveling down, I think we had
>>> better go ahead and remove the one remaining => operator from hstore
>>> in 9.2:

>> Fair enough.

> Would it then be added as an alias for := for named function parameters? Or would that come still later?

Once we do that, it will be impossible not merely deprecated to use =>
as an operator name. I think that has to wait at least another release
cycle or two past where we're using it ourselves.

regards, tom lane

On Oct 6, 2011, at 10:37 AM, Tom Lane wrote:

>> Would it then be added as an alias for := for named function parameters? Or would that come still later?
>
> Once we do that, it will be impossible not merely deprecated to use =>
> as an operator name. I think that has to wait at least another release
> cycle or two past where we're using it ourselves.

Okay. I kind of like := so there's no rush AFAIC. :-)

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>> Would it then be added as an alias for := for named function parameters? Or would that come still later?

>> Once we do that, it will be impossible not merely deprecated to use =>
>> as an operator name. I think that has to wait at least another release
>> cycle or two past where we're using it ourselves.

> Okay. I kind of like := so there's no rush AFAIC. :-)

Hmm ... actually, that raises another issue that I'm not sure whether
there's consensus for or not. Are we intending to keep name := value
syntax forever, as an alternative to the standard name => value syntax?
I can't immediately see a reason not to, other than the "it's not
standard" argument.

Because if we *are* going to keep it forever, there's no very good
reason why we shouldn't accept this plpgsql cursor patch now. We'd
just have to remember to extend plpgsql to take => at the same time
we do that for core function calls.

regards, tom lane

On Oct 6, 2011, at 10:46 AM, Tom Lane wrote:

>> Okay. I kind of like := so there's no rush AFAIC. :-)
>
> Hmm ... actually, that raises another issue that I'm not sure whether
> there's consensus for or not. Are we intending to keep name := value
> syntax forever, as an alternative to the standard name => value syntax?
> I can't immediately see a reason not to, other than the "it's not
> standard" argument.

The only reason it would be required, I think, is if the SQL standard developed some other use for that operator.

> Because if we *are* going to keep it forever, there's no very good
> reason why we shouldn't accept this plpgsql cursor patch now. We'd
> just have to remember to extend plpgsql to take => at the same time
> we do that for core function calls.

Makes sense.

Best,

David

On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>>> Would it then be added as an alias for := for named function parameters? Or would that come still later?
>
>>> Once we do that, it will be impossible not merely deprecated to use =>
>>> as an operator name.  I think that has to wait at least another release
>>> cycle or two past where we're using it ourselves.
>
>> Okay. I kind of like := so there's no rush AFAIC. :-)
>
> Hmm ... actually, that raises another issue that I'm not sure whether
> there's consensus for or not.  Are we intending to keep name := value
> syntax forever, as an alternative to the standard name => value syntax?
> I can't immediately see a reason not to, other than the "it's not
> standard" argument.
>
> Because if we *are* going to keep it forever, there's no very good
> reason why we shouldn't accept this plpgsql cursor patch now.  We'd
> just have to remember to extend plpgsql to take => at the same time
> we do that for core function calls.

It's hard to see adding support for => and dropping support for := in
the same release. That would be a compatibility nightmare.

If := is used by the standard for some other, incompatible purpose,
then I suppose we would want to add support for =>, wait a few
releases, deprecate :=, wait a couple of releases, remove :=
altogether. But IIRC we picked := precisely because the standard
didn't use it at all, or at least not for anything related... in which
case we may as well keep it around more or less indefinitely.

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

2011/10/6 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>>>> Would it then be added as an alias for := for named function parameters? Or would that come still later?
>>
>>>> Once we do that, it will be impossible not merely deprecated to use =>
>>>> as an operator name.  I think that has to wait at least another release
>>>> cycle or two past where we're using it ourselves.
>>
>>> Okay. I kind of like := so there's no rush AFAIC. :-)
>>
>> Hmm ... actually, that raises another issue that I'm not sure whether
>> there's consensus for or not.  Are we intending to keep name := value
>> syntax forever, as an alternative to the standard name => value syntax?
>> I can't immediately see a reason not to, other than the "it's not
>> standard" argument.
>>
>> Because if we *are* going to keep it forever, there's no very good
>> reason why we shouldn't accept this plpgsql cursor patch now.  We'd
>> just have to remember to extend plpgsql to take => at the same time
>> we do that for core function calls.
>
> It's hard to see adding support for => and dropping support for := in
> the same release.  That would be a compatibility nightmare.
>
> If := is used by the standard for some other, incompatible purpose,
> then I suppose we would want to add support for =>, wait a few
> releases, deprecate :=, wait a couple of releases, remove :=
> altogether.  But IIRC we picked := precisely because the standard
> didn't use it at all, or at least not for anything related... in which
> case we may as well keep it around more or less indefinitely.

+1

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Forgive my ignorance -- do I need to be doing anything else now seeing as I started the review?

On 07/10/2011, at 7:15 AM, Pavel Stehule wrote:

> 2011/10/6 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>>>>> Would it then be added as an alias for := for named function parameters? Or would that come still later?
>>>
>>>>> Once we do that, it will be impossible not merely deprecated to use =>
>>>>> as an operator name. I think that has to wait at least another release
>>>>> cycle or two past where we're using it ourselves.
>>>
>>>> Okay. I kind of like := so there's no rush AFAIC. :-)
>>>
>>> Hmm ... actually, that raises another issue that I'm not sure whether
>>> there's consensus for or not. Are we intending to keep name := value
>>> syntax forever, as an alternative to the standard name => value syntax?
>>> I can't immediately see a reason not to, other than the "it's not
>>> standard" argument.
>>>
>>> Because if we *are* going to keep it forever, there's no very good
>>> reason why we shouldn't accept this plpgsql cursor patch now. We'd
>>> just have to remember to extend plpgsql to take => at the same time
>>> we do that for core function calls.
>>
>> It's hard to see adding support for => and dropping support for := in
>> the same release. That would be a compatibility nightmare.
>>
>> If := is used by the standard for some other, incompatible purpose,
>> then I suppose we would want to add support for =>, wait a few
>> releases, deprecate :=, wait a couple of releases, remove :=
>> altogether. But IIRC we picked := precisely because the standard
>> didn't use it at all, or at least not for anything related... in which
>> case we may as well keep it around more or less indefinitely.
>
> +1
>
> Pavel
>
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On 2011-10-06 16:04, Royce Ausburn wrote:
> Initial Review for patch:
>
> http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php

Hello Royce,

Thank you for your review.

>
> I don't think so. The new feature accepts opening a cursor with some
> parameter names not specified:
>
> open cur1(param3 := 4, 1, param1 := 5);
>
> It seems that if a parameter is not named, its position is used to
> bind to a variable. For example, the following fail:
>
> psql:plsqltest.sql:26: ERROR: cursor "cur1" argument 2 "param2"
> provided multiple times
> LINE 10: open cur1(param3 := 4, 1, param2 := 5);
>
> and
>
> psql:plsqltest.sql:26: ERROR: cursor "cur1" argument 2 "param2"
> provided multiple times
> LINE 10: open cur1(param2 := 4, 2, param1 := 5);
>
>
> I think that postgres ought to enforce some consistency here. Use one
> way or the other, never both.

This was meant as a feature, but I can remove it.
>
>
> I can also produce some unhelpful errors when I give bad syntax. For
> example:
>
> psql:plsqltest.sql:28: ERROR: cursor "cur1" argument 1 "param1"
> provided multiple times
> LINE 11: open cur1( param3 : = 4, 2, param1 := 5);
> (notice the space between the : and =)

Yes, the whole of the expression before the first comma, 'param3 : = 4'
is not recognized as <parametername> <:= symbol> <expression>, so that
is taken as the value of the first parameter. This value is parsed after
all named arguments are read, and hence no meaningful error is given. If
there was no param1 parameter name at the end, the 'multiple times'
error would not have caused the processing to stop, and a syntax error
at the correct : would have been given.

The same reasoning also explains the other 'multiple times' errors you
could get, by putting a syntax error in some value.
>
> --
>
> open cur1( param3 := param3 , param2 = 3, param1 := 1 );
>
> psql:plsqltest.sql:29: ERROR: column "param2" does not exist
> LINE 2: ,param2 = 3
> ^
> QUERY: SELECT 1
> ,param2 = 3
> ,param3;
> CONTEXT: PL/pgSQL function "named_para_test" line 7 at OPEN

This is a valid error, since the parser / SQL will try to evaluate the
boolean expression param2 = 3, while param2 is not a defined variabele.

Again, thank you very much for your thorough review. I'll update the
patch so mixing positional and named parameters are removed, add
documentation, and give syntax errors before an error message indicating
that positional and named parameters were mixed.

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

On 2011-10-07 12:21, Yeb Havinga wrote:
> On 2011-10-06 16:04, Royce Ausburn wrote:
>> Initial Review for patch:
>>
>> http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php
>
>
> Again, thank you very much for your thorough review. I'll update the
> patch so mixing positional and named parameters are removed, add
> documentation, and give syntax errors before an error message
> indicating that positional and named parameters were mixed.
>

Attach is v2 of the patch.

Mixed notation now raises an error.

In contrast with what I said above, named parameter related errors are
thrown before any syntax errors. I tested with raising syntax errors
first but the resulting code was a bit more ugly and the sql checking
under a error condition (i.e. double named parameter error means there
is one parameter in short) was causing serious errors.

Documentation was also added, regression tests updated.

regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

On 08/10/2011, at 1:56 AM, Yeb Havinga wrote:

> Attach is v2 of the patch.
>
> Mixed notation now raises an error.
>
> In contrast with what I said above, named parameter related errors are thrown before any syntax errors. I tested with raising syntax errors first but the resulting code was a bit more ugly and the sql checking under a error condition (i.e. double named parameter error means there is one parameter in short) was causing serious errors.
>
> Documentation was also added, regression tests updated.

I've tested this patch out and can confirm mixed notation produces an error:

psql:plsqltest.sql:27: ERROR: mixing positional and named parameter assignment not allowed in cursor "cur1"
LINE 10: open cur1( param2 := 4, 2, 5);

Just one small thing: it'd be nice to have an example for cursor declaration with named parameters. Your patch adds one for opening a cursor, but not for declaring one.

Other than that, I think the patch is good. Everything works as advertised =)

--Royce

Hello Royce,

Thanks again for testing.

On 2011-10-11 13:55, Royce Ausburn wrote:

> Just one small thing: it'd be nice to have an example for cursor declaration with named parameters. Your patch adds one for opening a cursor, but not for declaring one.

Declaration of cursors with named parameters is already part of
PostgreSQL (so it is possible to use the parameter names in the cursor
query instead of $1, $2, etc.) and it also already documented with an
example, just a few lines above the open examples. See curs3 on
http://developer.postgresql.org/pgdocs/postgres/plpgsql-cursors.html

> Other than that, I think the patch is good. Everything works as advertised =)

Thanks!

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

On 11/10/2011, at 11:38 PM, Yeb Havinga wrote:

> Declaration of cursors with named parameters is already part of PostgreSQL (so it is possible to use the parameter names in the cursor query instead of $1, $2, etc.) and it also already documented with an example, just a few lines above the open examples. See curs3 on http://developer.postgresql.org/pgdocs/postgres/plpgsql-cursors.html

Doh - my apologies!

Yeb Havinga <yebhavinga(at)gmail(dot)com> writes:
> Hello Royce,
> Thanks again for testing.

I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.

The biggest problem is that the patch cuts up and reassembles the source
text and then hands that off to check_sql_expr, ignoring the latter's
comment that says

* It is assumed that "stmt" represents a copy of the function source text
* beginning at offset "location", with leader text of length "leaderlen"
* (typically "SELECT ") prefixed to the source text. We use this assumption
* to transpose any error cursor position back to the function source text.

This means that syntax error positioning for errors in the argument
expressions is generally pretty wacko, but especially so if the
arguments are supplied in other than left-to-right order. An example is

create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p2 := 42/, p1 := 77);
end $$ language plpgsql;

which gives this:

ERROR: syntax error at or near ";"
LINE 6: open c1 ( p2 := 42/, p1 := 77);
^

which is not going to impress anybody as helpful, either as to the message
text (the user didn't write any ";" nearby) or as to the cursor
positioning. And it doesn't look very much more professional if the error
is run-time rather than parse-time:

create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p2 := 42/0, p1 := 77);
end $$ language plpgsql;

select fooey();
ERROR: division by zero
CONTEXT: SQL statement "SELECT 77
,42/0;"
PL/pgSQL function "fooey" line 6 at OPEN

where again you're displaying something almost completely unlike what the
user wrote.

I don't have any great suggestions about how to fix this :-(. Perhaps
it'd be acceptable to copy the argument-list text into the query string,
carefully replacing the parameters and := marks with appropriate amounts
of whitespace (the same number of characters, not the same number of
bytes). This would allow syntax errors from check_sql_expr to match up
correctly, and runtime errors would at least show a string that doesn't
look totally unlike what the user wrote. But it would be painful to get
the values assigned to the cursor parameters in the right order --- I
think most likely you'd need to generate a new "row" list having the
cursor parameter values in the order written in the OPEN.

Also, I concur with Royce that it would be a whole lot better to apply
check_sql_expr to each argument expression as soon as you've parsed it
off, so that typos like "p1 : = 42" get detected soon enough to be
helpful, rather than ending up with misleading error messages. Very
possibly that would also simplify getting parse-time syntax errors to
point to the right places, since you'd be calling check_sql_expr on text
not far separated from the source query. (In fact, personally I'd use
read_sql_expression2(',', ')', ...) to parse off each expression and check
it immediately.) Maybe with that fix, it'd be all right if the run-time
query rearranged the expressions into the order required by the cursor
... though I'd still counsel spending more sweat on making the run-time
string look nicer. Something like

ERROR: division by zero
CONTEXT: SQL statement "SELECT /* p1 := */ 77 , /* p2 := */ 42/0"
PL/pgSQL function "fooey" line 6 at OPEN

would be easier for users to make sense of, I think.

By accident I noticed that the parameter name matching is inaccurate,
for instance this fails to fail:

create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p1 := 42, p2z := 77);
end $$ language plpgsql;

select fooey();

I think this is because of the use of strncmp() --- is that really needed
rather than just plain strcmp()?

Cursor positioning for some errors is a bit flaky, observe these cases:

ERROR: mixing positional and named parameter assignment not allowed in cursor "c1"
LINE 6: open c1 ( p2 : = 42, p2 := 77);
^

ERROR: cursor "c1" argument 2 "p2" provided multiple times
LINE 6: open c1 ( p2 := 42, p2 := 77);
^

In both of these cases I'd have expected the syntax cursor to point at
the second "p2". I'd lose the "2" in that second message text, as well
--- the cursor argument name is sufficient, and as-is it does not read
very nicely.

On the documentation front, the patch includes a hunk that changes the
description of DECLARE to claim that the argument names are optional,
something I see no support for in the code. It also fails to document
that this patch affects the behavior of cursor FOR loops as well as OPEN,
since both of those places use read_cursor_args().

regards, tom lane

On 2011-10-15 07:41, Tom Lane wrote:
> Yeb Havinga<yebhavinga(at)gmail(dot)com> writes:
>> Hello Royce,
>> Thanks again for testing.
> I looked this patch over but concluded that it's not ready to apply,
> mainly because there are too many weird behaviors around error
> reporting.

Tom, thanks for reviewing - getting the syntax errors to be at the exact
location was indeed something that I thought would be near impossible,
however the whitespace suggestion together with the others you made seem
like a good path to go forward. Thanks for taking the time to write your
comments, it will be a great help with making an improved version.

regards,
Yeb Havinga

On 2011-10-15 07:41, Tom Lane wrote:
> Yeb Havinga<yebhavinga(at)gmail(dot)com> writes:
>> Hello Royce,
>> Thanks again for testing.
> I looked this patch over but concluded that it's not ready to apply,
> mainly because there are too many weird behaviors around error
> reporting.

Thanks again for the review and comments. Attached is v3 of the patch
that addresses all of the points made by Tom. In the regression test I
added a section under --- START ADDITIONAL TESTS that might speedup testing.

> On the documentation front, the patch includes a hunk that changes the
> description of DECLARE to claim that the argument names are optional,
> something I see no support for in the code. It also fails to document
> that this patch affects the behavior of cursor FOR loops as well as OPEN,
> since both of those places use read_cursor_args().

The declare section was removed. The cursor for loop section was changed
to include a reference to named parameters, however I was unsure about
OPEN as I was under the impression that was already altered.

regards,
Yeb Havinga

On 2011-11-14 15:45, Yeb Havinga wrote:
> On 2011-10-15 07:41, Tom Lane wrote:
>> Yeb Havinga<yebhavinga(at)gmail(dot)com> writes:
>>> Hello Royce,
>>> Thanks again for testing.
>> I looked this patch over but concluded that it's not ready to apply,
>> mainly because there are too many weird behaviors around error
>> reporting.
>
> Thanks again for the review and comments. Attached is v3 of the patch
> that addresses all of the points made by Tom. In the regression test I
> added a section under --- START ADDITIONAL TESTS that might speedup
> testing.

Please disregard the previous patch: besides that it contained an unused
function, it turned out my statement that all of Tom's points were
addressed was not true - the attached patch fixes the remaining issue of
putting two kinds of errors at the correct start of the current argument
location.

I also put some more comments in the regression test section: mainly to
assist providing testcases for review, not for permanent inclusion.

To address a corner case of the form 'p1 := 1 -- comments\n, p2 := 2' it
was necessary to have read_sql_construct not trim trailing whitespace,
since that results in an expression of the form '1 -- comments, 2' which
is wrong.

regards,
Yeb Havinga