Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
On 08/07/2014 01:11 AM, Marko Tiikkaja wrote: On 7/21/14, 10:56 PM, I wrote: Here's a patch which allows you to notice those annoying bugs with INTO slightly more quickly. Adding to the next commit phest. New version, fixed bugs with set operations and VALUES lists. Looks good. It seems weird to pass a PLpgSQL_row struct to check_sql_expr. check_sql_expr only needs to know how many attributes is expected to be in the target list, so it would be more natural to just pass an int expected_natts. Once you do that, you could trivially also add checking for other cases, e.g: do $$ declare i int4; begin -- fails at runtime, because SELECT 1,3 returns two attributes, -- but FOR expects 1 for i in 1,3..(2) loop raise notice 'foo %', i; end loop; end; $$; There's probably more checking like that that you could add, but that can be done as add-on patches, if ever. The INTO mistake happens a lot more easily. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
On 8/21/14, 1:19 PM, Heikki Linnakangas wrote: On 08/07/2014 01:11 AM, Marko Tiikkaja wrote: On 7/21/14, 10:56 PM, I wrote: Here's a patch which allows you to notice those annoying bugs with INTO slightly more quickly. Adding to the next commit phest. New version, fixed bugs with set operations and VALUES lists. Looks good. There's probably more checking like that that you could add, but that can be done as add-on patches, if ever. The INTO mistake happens a lot more easily. Yeah, I think the mistake is at least as easy to do in FOR .. IN query, and I'm planning to add checks for that as well. But I haven't found the time to look at it amongst all the other patches and projects I have going (and also, unfortunately, I'm on vacation right now). It seems weird to pass a PLpgSQL_row struct to check_sql_expr. check_sql_expr only needs to know how many attributes is expected to be in the target list, so it would be more natural to just pass an int expected_natts. I'm not sure about this, though. AFAICT all the interesting cases are already holding a PLpgSQL_row, and in that case it seems easier to just pass that in to check_sql_expr() without making the callers worry about extracting the expected_natts from the row. And we can always change the interface should such a case come up, since the interface is completely internal. Just my 0.02EUR, of course. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
On 08/21/2014 02:09 PM, Marko Tiikkaja wrote: On 8/21/14, 1:19 PM, Heikki Linnakangas wrote: On 08/07/2014 01:11 AM, Marko Tiikkaja wrote: On 7/21/14, 10:56 PM, I wrote: Here's a patch which allows you to notice those annoying bugs with INTO slightly more quickly. Adding to the next commit phest. New version, fixed bugs with set operations and VALUES lists. Looks good. There's probably more checking like that that you could add, but that can be done as add-on patches, if ever. The INTO mistake happens a lot more easily. Yeah, I think the mistake is at least as easy to do in FOR .. IN query, and I'm planning to add checks for that as well. But I haven't found the time to look at it amongst all the other patches and projects I have going Ok. (and also, unfortunately, I'm on vacation right now). Oh, have fun! It seems weird to pass a PLpgSQL_row struct to check_sql_expr. check_sql_expr only needs to know how many attributes is expected to be in the target list, so it would be more natural to just pass an int expected_natts. I'm not sure about this, though. AFAICT all the interesting cases are already holding a PLpgSQL_row, and in that case it seems easier to just pass that in to check_sql_expr() without making the callers worry about extracting the expected_natts from the row. Hmm. The integer FOR syntax I used in my example does not, it always expects 1 output column. And we can always change the interface should such a case come up, since the interface is completely internal. Just my 0.02EUR, of course. You might want to add a helper function to count the number of attributes in a PLpgSQL_row. Then the check_sql_expr call would be almost as simple: check_sql_expr(..., get_row_natts(row)). - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
On 7/21/14, 10:56 PM, I wrote: Here's a patch which allows you to notice those annoying bugs with INTO slightly more quickly. Adding to the next commit phest. New version, fixed bugs with set operations and VALUES lists. .marko *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** *** 4719,4725 a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ varnameplpgsql.extra_errors/ for errors. Both can be set either to a comma-separated list of checks, literalnone/ or literalall/. The default is literalnone/. Currently the list of available checks ! includes only one: variablelist varlistentry termvarnameshadowed_variables/varname/term --- 4719,4725 varnameplpgsql.extra_errors/ for errors. Both can be set either to a comma-separated list of checks, literalnone/ or literalall/. The default is literalnone/. Currently the list of available checks ! is as follows: variablelist varlistentry termvarnameshadowed_variables/varname/term *** *** 4729,4734 a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ --- 4729,4744 /para /listitem /varlistentry +varlistentry + termvarnamenum_into_expressions/varname/term + listitem + para + When possible, checks that the number of expressions specified in the + SELECT or RETURNING list of a query matches the number expected by the + INTO target. This check is done on a best effort basis. + /para + /listitem +/varlistentry /variablelist The following example shows the effect of varnameplpgsql.extra_warnings/ *** a/src/pl/plpgsql/src/pl_gram.y --- b/src/pl/plpgsql/src/pl_gram.y *** *** 22,27 --- 22,28 #include parser/scanner.h #include parser/scansup.h #include utils/builtins.h + #include nodes/nodefuncs.h /* Location tracking support --- simpler than bison's default */ *** *** 97,103 static PLpgSQL_row *make_scalar_list1(char *initial_name, PLpgSQL_datum *initial_datum, int lineno, int location); staticvoid check_sql_expr(const char *stmt, int location, ! int leaderlen); staticvoid plpgsql_sql_error_callback(void *arg); staticPLpgSQL_type*parse_datatype(const char *string, int location); staticvoid check_labels(const char *start_label, --- 98,104 PLpgSQL_datum *initial_datum, int lineno, int location); staticvoid check_sql_expr(const char *stmt, int location, ! int leaderlen, PLpgSQL_row *check_row); staticvoid plpgsql_sql_error_callback(void *arg); staticPLpgSQL_type*parse_datatype(const char *string, int location); staticvoid check_labels(const char *start_label, *** *** 106,111 static void check_labels(const char *start_label, --- 107,115 staticPLpgSQL_expr*read_cursor_args(PLpgSQL_var *cursor, int until, const char *expected); staticList*read_raise_options(void); + staticboolfind_a_star_walker(Node *node, void *context); + staticint tlist_result_column_count(Node *stmt); + %} *** *** 1438,1444 for_control : for_variable K_IN PLpgSQL_stmt_fori *new; /* Check first expression is well-formed */ ! check_sql_expr(expr1-query, expr1loc, 7); /* Read and check the second one */ expr2 = read_sql_expression2(K_LOOP, K_BY, --- 1442,1448 PLpgSQL_stmt_fori *new; /* Check first expression is well-formed */ ! check_sql_expr(expr1-query, expr1loc, 7, NULL);
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
On 7/22/14, 7:06 AM, Pavel Stehule wrote: I looked on this patch and I am thinking so it is not a good idea. It introduce early dependency between functions and pg_class based objects. What dependency? The patch only looks at the raw parser output, so it won't e.g. know whether SELECT * INTO a, b FROM foo; is problematic or not. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
2014-07-22 8:52 GMT+02:00 Marko Tiikkaja ma...@joh.to: On 7/22/14, 7:06 AM, Pavel Stehule wrote: I looked on this patch and I am thinking so it is not a good idea. It introduce early dependency between functions and pg_class based objects. What dependency? The patch only looks at the raw parser output, so it won't e.g. know whether SELECT * INTO a, b FROM foo; is problematic or not. I am sorry, I was confused There is dependencty in variable type, but this dependency is not new. Regards Pavel .marko
Re: [HACKERS] plpgsql.extra_warnings='num_into_expressions'
Hi I looked on this patch and I am thinking so it is not a good idea. It introduce early dependency between functions and pg_class based objects. This check should not be integrated to function validation directly. We can integrate it to plpgsql_check Regards Pavel 2014-07-21 22:56 GMT+02:00 Marko Tiikkaja ma...@joh.to: Hi again, Here's a patch which allows you to notice those annoying bugs with INTO slightly more quickly. Adding to the next commit phest. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
