Yeb Havinga <yebhavi...@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 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers