Re: [REVIEW] Patch for cursor calling with named parameters - Mailing list pgsql-hackers

Yeb Havinga <yebhavinga@gmail.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
textof length "leaderlen"* (typically "SELECT ") prefixed to the source text.  We use this assumption* to transpose any
errorcursor 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