Hi 2018-01-07 0:59 GMT+01:00 Stephen Frost <sfr...@snowman.net>:
> Greetings Pavel, > > * Pavel Stehule (pavel.steh...@gmail.com) wrote: > > 2017-11-30 3:44 GMT+01:00 Michael Paquier <michael.paqu...@gmail.com>: > > > At least documentation needs patching, or this is going to generate > > > warnings on HEAD at compilation. I am moving this to next CF for lack > > > of reviews, and the status is waiting on author as this needs at least > > > a couple of doc fixes. > > > > fixed doc attached > > Looks like this patch should have been in "Needs Review" state, not > "Waiting for author", since it does apply, build and pass make > check-world, but as I'm signed up to review it, I'll do so here: > > diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml > index 7d23ed437e..efa48bc13c 100644 > --- a/doc/src/sgml/plpgsql.sgml > +++ b/doc/src/sgml/plpgsql.sgml > @@ -4963,6 +4963,11 @@ a_output := a_output || $$ if v_$$ || > referrer_keys.kind || $$ like '$$ > so you are advised to test in a separate development environment. > </para> > > + <para> > + The setting <varname>plpgsql.extra_warnings</varname> to > <literal>all</literal> is a > + good idea in developer or test environments. > + </para> > > Better language for this would be: > > Setting <varname>plpgsql.extra_warnings</varname>, or > <varname>plpgsql.extra_errors</varname>, as appropriate, to > <literal>all</literal> is encouraged in development and/or testing > environments. > > @@ -4979,6 +4984,30 @@ a_output := a_output || $$ if v_$$ || > referrer_keys.kind || $$ like '$$ > </para> > </listitem> > </varlistentry> > + > + <varlistentry> > + <term><varname>strict_multi_assignment</varname></term> > + <listitem> > + <para> > + Some <application>PL/PgSQL</application> commands allows to assign > a values to > + more than one variable. The number of target variables should not be > + equal to number of source values. Missing values are replaced by > NULL > + value, spare values are ignored. More times this situation signalize > + some error. > + </para> > + </listitem> > + </varlistentry> > > Better language: > > Some <application>PL/PgSQL</application> commands allow assigning values > to more than one variable at a time, such as SELECT INTO. Typically, > the number of target variables and the number of source variables should > match, though <application>PL/PgSQL</application> will use NULL for > missing values and extra variables are ignored. Enabling this check > will cause <application>PL/PgSQL</application> to throw a WARNING or > ERROR whenever the number of target variables and the number of source > variables are different. > > + <varlistentry> > + <term><varname>too_many_rows</varname></term> > + <listitem> > + <para> > + When result is assigned to a variable by <literal>INTO</literal> > clause, > + checks if query returns more than one row. In this case the > assignment > + is not deterministic usually - and it can be signal some issues in > design. > + </para> > + </listitem> > + </varlistentry> > </variablelist> > > Better language: > > Enabling this check will cause <application>PL/PgSQL</application> to > check if a given query returns more than one row when an > <literal>INTO</literal> clause is used. As an INTO statement will only > ever use one row, having a query return multiple rows is generally > either inefficient and/or nondeterministic and therefore is likely an > error. > > @@ -4997,6 +5026,34 @@ WARNING: variable "f1" shadows a previously > defined variable > LINE 3: f1 int; > ^ > CREATE FUNCTION > +</programlisting> > + > + The another example shows the effect of <varname>plpgsql.extra_ > warnings</varname> > + set to <varname>strict_multi_assignment</varname>: > +<programlisting> > > Better language: > > The below example shows the effects of setting > <varname>plpgsql.extra_warnings</varname> to > <varname>strict_multi_assignment</varname>: > > diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c > index ec480cb0ba..0879e84cd2 100644 > --- a/src/pl/plpgsql/src/pl_exec.c > +++ b/src/pl/plpgsql/src/pl_exec.c > @@ -3629,6 +3629,24 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, > long tcount; > int rc; > PLpgSQL_expr *expr = stmt->sqlstmt; > + bool too_many_rows_check; > + int too_many_rows_level; > + > + if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS) > + { > + too_many_rows_check = true; > + too_many_rows_level = ERROR; > + } > + else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS) > + { > + too_many_rows_check = true; > + too_many_rows_level = WARNING; > + } > + else > + { > + too_many_rows_check = false; > + too_many_rows_level = NOTICE; > + } > > > I'm not sure why we need two variables here- couldn't we simply look at > too_many_rows_level? eg: too_many_rows_level >= WARNING ? ... > > Not as big a deal, but I would change it to be 'check_too_many_rows' as > a variable name too. > > @@ -3678,7 +3696,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, > */ > if (stmt->into) > { > - if (stmt->strict || stmt->mod_stmt) > + if (stmt->strict || stmt->mod_stmt || too_many_rows_check) > tcount = 2; > else > tcount = 1; > > The comment above this block needs updating for this change and, in > general, there's probably other pieces of code that this patch > changes where the comments should either be improved or ones added. > > > @@ -6033,12 +6051,48 @@ exec_move_row(PLpgSQL_execstate *estate, > int t_natts; > int fnum; > int anum; > + bool strict_multiassignment_check; > + int strict_multiassignment_level; > + > + if (plpgsql_extra_errors & PLPGSQL_XCHECK_ > STRICTMULTIASSIGNMENT) > + { > + strict_multiassignment_check = true; > + strict_multiassignment_level = ERROR; > + } > + else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_ > STRICTMULTIASSIGNMENT) > + { > + strict_multiassignment_check = true; > + strict_multiassignment_level = WARNING; > + } > + else > + { > + strict_multiassignment_check = false; > + strict_multiassignment_level = NOTICE; > + } > > Same comments for this, more-or-less, as the above sections. > > if (HeapTupleIsValid(tup)) > t_natts = HeapTupleHeaderGetNatts(tup->t_data); > else > t_natts = 0; > > + if (strict_multiassignment_check) > + { > + int i; > + > + anum = 0; > + for (i = 0; i < td_natts; i++) > + if (!TupleDescAttr(tupdesc, > i)->attisdropped) > + anum++; > + > + if (anum != row->nfields) > + { > + ereport(strict_multiassignment_level, > + (errcode(ERRCODE_DATATYPE_ > MISMATCH), > + errmsg("Number of > evaluated attributies (%d) does not match expected attributies (%d)", > + > anum, row->nfields))); > + } > + } > > I would have thought you'd incorporate this into the loop below instead > of adding a whole new section..? At the least, this should include > better comments, and isn't there an issue here where you aren't > accounting for dropped columns in row structs? See the comments in the > loop below this. > > I'd suggest you try to construct a case which (incorrectly) throws a > warning for a row struct with a dropped column with your current patch > and then add that to the regression tests too, since it appears to have > been missed. > > There, now it's in the correct Waiting for Author state. :) > thank you for comments. All should be fixed in attached patch Regards Pavel > Thanks! > > Stephen >
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index 7d23ed437e..af1cf4ab95 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -4951,7 +4951,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ </sect2> <sect2 id="plpgsql-extra-checks"> - <title>Additional Compile-time Checks</title> + <title>Additional Compile-time and Run-time Checks</title> <para> To aid the user in finding instances of simple but common problems before @@ -4963,26 +4963,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ so you are advised to test in a separate development environment. </para> - <para> - These additional checks are enabled through the configuration variables - <varname>plpgsql.extra_warnings</varname> for warnings and - <varname>plpgsql.extra_errors</varname> for errors. Both can be set either to - a comma-separated list of checks, <literal>"none"</literal> or <literal>"all"</literal>. - The default is <literal>"none"</literal>. Currently the list of available checks - includes only one: - <variablelist> - <varlistentry> - <term><varname>shadowed_variables</varname></term> - <listitem> - <para> - Checks if a declaration shadows a previously defined variable. - </para> - </listitem> - </varlistentry> - </variablelist> + <para> + Setting <varname>plpgsql.extra_warnings</varname>, or + <varname>plpgsql.extra_errors</varname>, as appropriate, to <literal>all</literal> + is encouraged in development and/or testing environments. + </para> + + <para> + These additional checks are enabled through the configuration variables + <varname>plpgsql.extra_warnings</varname> for warnings and + <varname>plpgsql.extra_errors</varname> for errors. Both can be set either to + a comma-separated list of checks, <literal>"none"</literal> or + <literal>"all"</literal>. The default is <literal>"none"</literal>. Currently + the list of available checks includes only one: + <variablelist> + <varlistentry> + <term><varname>shadowed_variables</varname></term> + <listitem> + <para> + Checks if a declaration shadows a previously defined variable. + </para> + </listitem> + </varlistentry> - The following example shows the effect of <varname>plpgsql.extra_warnings</varname> - set to <varname>shadowed_variables</varname>: + <varlistentry> + <term><varname>strict_multi_assignment</varname></term> + <listitem> + <para> + Some <application>PL/PgSQL</application> commands allow assigning values + to more than one variable at a time, such as SELECT INTO. Typically, + the number of target variables and the number of source variables should + match, though <application>PL/PgSQL</application> will use NULL for + missing values and extra variables are ignored. Enabling this check + will cause <application>PL/PgSQL</application> to throw a WARNING or + ERROR whenever the number of target variables and the number of source + variables are different. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><varname>too_many_rows</varname></term> + <listitem> + <para> + Enabling this check will cause <application>PL/PgSQL</application> to + check if a given query returns more than one row when an + <literal>INTO</literal> clause is used. As an INTO statement will only + ever use one row, having a query return multiple rows is generally + either inefficient and/or nondeterministic and therefore is likely an + error. + </para> + </listitem> + </varlistentry> + </variablelist> + + The following example shows the effect of <varname>plpgsql.extra_warnings</varname> + set to <varname>shadowed_variables</varname>: <programlisting> SET plpgsql.extra_warnings TO 'shadowed_variables'; @@ -4998,8 +5034,38 @@ LINE 3: f1 int; ^ CREATE FUNCTION </programlisting> - </para> - </sect2> + The below example shows the effects of setting + <varname>plpgsql.extra_warnings</varname> to + <varname>strict_multi_assignment</varname>: +<programlisting> +SET plpgsql.extra_warnings TO 'strict_multi_assignment'; + +CREATE OR REPLACE FUNCTION public.foo() + RETURNS void + LANGUAGE plpgsql +AS $$ +DECLARE + x int; + y int; +BEGIN + SELECT 1 INTO x, y; + SELECT 1, 2 INTO x, y; + SELECT 1, 2, 3 INTO x, y; +END; +$$ + +SELECT foo(); +WARNING: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_warnings is active. +WARNING: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_warnings is active. + foo +----- + +(1 row) +</programlisting> + </para> + </sect2> </sect1> <!-- **** Porting from Oracle PL/SQL **** --> diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index d096f242cd..2ce51213f1 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -3559,6 +3559,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, long tcount; int rc; PLpgSQL_expr *expr = stmt->sqlstmt; + int too_many_rows_level = 0; + + if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS) + too_many_rows_level = ERROR; + else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS) + too_many_rows_level = WARNING; /* * On the first call for this statement generate the plan, and detect @@ -3597,9 +3603,10 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, /* * If we have INTO, then we only need one row back ... but if we have INTO - * STRICT, ask for two rows, so that we can verify the statement returns - * only one. INSERT/UPDATE/DELETE are always treated strictly. Without - * INTO, just run the statement to completion (tcount = 0). + * STRICT or extra check too_many_rows, ask for two rows, so that we can + * verify the statement returns only one. INSERT/UPDATE/DELETE are always + * treated strictly. Without INTO, just run the statement to completion + * (tcount = 0). * * We could just ask for two rows always when using INTO, but there are * some cases where demanding the extra row costs significant time, eg by @@ -3608,7 +3615,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, */ if (stmt->into) { - if (stmt->strict || stmt->mod_stmt) + if (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING) tcount = 2; else tcount = 1; @@ -3722,19 +3729,26 @@ exec_stmt_execsql(PLpgSQL_execstate *estate, } else { - if (n > 1 && (stmt->strict || stmt->mod_stmt)) + if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING)) { char *errdetail; + int errlevel; + bool use_errhint; if (estate->func->print_strict_params) errdetail = format_expr_params(estate, expr); else errdetail = NULL; - ereport(ERROR, + errlevel = stmt->strict || stmt->mod_stmt ? ERROR : too_many_rows_level; + use_errhint = !(stmt->strict || stmt->mod_stmt); + + ereport(errlevel, (errcode(ERRCODE_TOO_MANY_ROWS), errmsg("query returned more than one row"), - errdetail ? errdetail_internal("parameters: %s", errdetail) : 0)); + errdetail ? errdetail_internal("parameters: %s", errdetail) : 0, + use_errhint ? errhint("too_many_rows check of extra_%s is active.", + too_many_rows_level == ERROR ? "errors" : "warnings") : 0)); } /* Put the first result row into the target */ exec_move_row(estate, target, tuptab->vals[0], tuptab->tupdesc); @@ -5944,6 +5958,12 @@ exec_move_row(PLpgSQL_execstate *estate, int t_natts; int fnum; int anum; + int strict_multiassignment_level = 0; + + if (plpgsql_extra_errors & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT) + strict_multiassignment_level = ERROR; + else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT) + strict_multiassignment_level = WARNING; if (HeapTupleIsValid(tup)) t_natts = HeapTupleHeaderGetNatts(tup->t_data); @@ -5974,6 +5994,7 @@ exec_move_row(PLpgSQL_execstate *estate, value = SPI_getbinval(tup, tupdesc, anum + 1, &isnull); else { + /* there are no data */ value = (Datum) 0; isnull = true; } @@ -5987,12 +6008,37 @@ exec_move_row(PLpgSQL_execstate *estate, isnull = true; valtype = UNKNOWNOID; valtypmod = -1; + + if (strict_multiassignment_level >= WARNING) + ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("Number of evaluated fields does not match expected."), + errhint("strict_multi_assignement check of extra_%s is active.", + strict_multiassignment_level == ERROR ? "errors" : "warnings"))); } exec_assign_value(estate, (PLpgSQL_datum *) var, value, isnull, valtype, valtypmod); } + /* + * When stric_multiassignment extra check is active, then ensure so there + * are not more unassigned columns. + */ + if (strict_multiassignment_level >= WARNING && anum < td_natts) + { + while (anum < td_natts && + TupleDescAttr(tupdesc, anum)->attisdropped) + anum++; /* skip dropped column in tuple */ + + if (anum < td_natts) + ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("Number of evaluated fields does not match expected."), + errhint("strict_multi_assignement check of extra_%s is active.", + strict_multiassignment_level == ERROR ? "errors" : "warnings"))); + } + return; } diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 4c2ba2f734..c6064299ab 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -92,6 +92,10 @@ plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source) if (pg_strcasecmp(tok, "shadowed_variables") == 0) extrachecks |= PLPGSQL_XCHECK_SHADOWVAR; + else if (pg_strcasecmp(tok, "too_many_rows") == 0) + extrachecks |= PLPGSQL_XCHECK_TOOMANYROWS; + else if (pg_strcasecmp(tok, "strict_multi_assignment") == 0) + extrachecks |= PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT; else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0) { GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index c571afa34b..e0ee6172db 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1019,7 +1019,9 @@ extern bool plpgsql_check_asserts; /* extra compile-time checks */ #define PLPGSQL_XCHECK_NONE 0 -#define PLPGSQL_XCHECK_SHADOWVAR 1 +#define PLPGSQL_XCHECK_SHADOWVAR (1 << 1) +#define PLPGSQL_XCHECK_TOOMANYROWS (1 << 2) +#define PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT (1 << 3) #define PLPGSQL_XCHECK_ALL ((int) ~0) extern int plpgsql_extra_warnings; diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 4783807ae0..5946903974 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3089,6 +3089,99 @@ select shadowtest(1); t (1 row) +-- runtime extra checks +set plpgsql.extra_warnings to 'too_many_rows'; +do $$ +declare x int; +begin + select v from generate_series(1,2) g(v) into x; +end; +$$; +WARNING: query returned more than one row +HINT: too_many_rows check of extra_warnings is active. +set plpgsql.extra_errors to 'too_many_rows'; +do $$ +declare x int; +begin + select v from generate_series(1,2) g(v) into x; +end; +$$; +ERROR: query returned more than one row +HINT: too_many_rows check of extra_errors is active. +CONTEXT: PL/pgSQL function inline_code_block line 4 at SQL statement +reset plpgsql.extra_errors; +reset plpgsql.extra_warnings; +set plpgsql.extra_warnings to 'strict_multi_assignment'; +do $$ +declare + x int; + y int; +begin + select 1 into x, y; + select 1,2 into x, y; + select 1,2,3 into x, y; +end +$$; +WARNING: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_warnings is active. +WARNING: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_warnings is active. +set plpgsql.extra_errors to 'strict_multi_assignment'; +do $$ +declare + x int; + y int; +begin + select 1 into x, y; + select 1,2 into x, y; + select 1,2,3 into x, y; +end +$$; +ERROR: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_errors is active. +CONTEXT: PL/pgSQL function inline_code_block line 6 at SQL statement +create table test_01(a int, b int, c int); +alter table test_01 drop column a; +do $$ +declare + x int; + y int; +begin + select * from test_01 into x, y; -- should be ok + raise notice 'ok'; + select * from test_01 into x; -- should to fail +end; +$$; +NOTICE: ok +ERROR: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_errors is active. +CONTEXT: PL/pgSQL function inline_code_block line 8 at SQL statement +do $$ +declare + t test_01; +begin + select 1, 2 into t; -- should be ok + raise notice 'ok'; + select 1, 2, 3 into t; -- should fail; +end; +$$; +NOTICE: ok +ERROR: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_errors is active. +CONTEXT: PL/pgSQL function inline_code_block line 7 at SQL statement +do $$ +declare + t test_01; +begin + select 1 into t; -- should fail; +end; +$$; +ERROR: Number of evaluated fields does not match expected. +HINT: strict_multi_assignement check of extra_errors is active. +CONTEXT: PL/pgSQL function inline_code_block line 5 at SQL statement +drop table test_01; +reset plpgsql.extra_errors; +reset plpgsql.extra_warnings; -- test scrollable cursor support create function sc_test() returns setof integer as $$ declare diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 768270d467..0e255b88f1 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -2605,6 +2605,92 @@ declare f1 int; begin return 1; end $$ language plpgsql; select shadowtest(1); +-- runtime extra checks +set plpgsql.extra_warnings to 'too_many_rows'; + +do $$ +declare x int; +begin + select v from generate_series(1,2) g(v) into x; +end; +$$; + +set plpgsql.extra_errors to 'too_many_rows'; + +do $$ +declare x int; +begin + select v from generate_series(1,2) g(v) into x; +end; +$$; + +reset plpgsql.extra_errors; +reset plpgsql.extra_warnings; + +set plpgsql.extra_warnings to 'strict_multi_assignment'; + +do $$ +declare + x int; + y int; +begin + select 1 into x, y; + select 1,2 into x, y; + select 1,2,3 into x, y; +end +$$; + +set plpgsql.extra_errors to 'strict_multi_assignment'; + +do $$ +declare + x int; + y int; +begin + select 1 into x, y; + select 1,2 into x, y; + select 1,2,3 into x, y; +end +$$; + +create table test_01(a int, b int, c int); + +alter table test_01 drop column a; + +do $$ +declare + x int; + y int; +begin + select * from test_01 into x, y; -- should be ok + raise notice 'ok'; + select * from test_01 into x; -- should to fail +end; +$$; + +do $$ +declare + t test_01; +begin + select 1, 2 into t; -- should be ok + raise notice 'ok'; + select 1, 2, 3 into t; -- should fail; +end; +$$; + +do $$ +declare + t test_01; +begin + select 1 into t; -- should fail; +end; +$$; + +drop table test_01; + +reset plpgsql.extra_errors; +reset plpgsql.extra_warnings; + -- test scrollable cursor support create function sc_test() returns setof integer as $$