diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index d6688e13f4..0bf76beb55 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -5034,7 +5034,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ - Additional Compile-time Checks + Additional Compile-time and Run-time Checks To aid the user in finding instances of simple but common problems before @@ -5046,26 +5046,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ so you are advised to test in a separate development environment. - - These additional checks are enabled through the configuration variables - plpgsql.extra_warnings for warnings and - plpgsql.extra_errors for errors. Both can be set either to - a comma-separated list of checks, "none" or "all". - The default is "none". Currently the list of available checks - includes only one: - - - shadowed_variables - - - Checks if a declaration shadows a previously defined variable. - - - - + + Setting plpgsql.extra_warnings, or + plpgsql.extra_errors, as appropriate, to "all" + is encouraged in development and/or testing environments. + + + + These additional checks are enabled through the configuration variables + plpgsql.extra_warnings for warnings and + plpgsql.extra_errors for errors. Both can be set either to + a comma-separated list of checks, "none" or + "all". The default is "none". Currently + the list of available checks includes: + + + shadowed_variables + + + Checks if a declaration shadows a previously defined variable. + + + - The following example shows the effect of plpgsql.extra_warnings - set to shadowed_variables: + + strict_multi_assignment + + + Some PL/PgSQL 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 PL/PgSQL will use NULL for + missing values and extra variables are ignored. Enabling this check + will cause PL/PgSQL to throw a WARNING or + ERROR whenever the number of target variables and the number of source + variables are different. + + + + + + too_many_rows + + + Enabling this check will cause PL/PgSQL to + check if a given query returns more than one row when an + INTO 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. + + + + + + The following example shows the effect of plpgsql.extra_warnings + set to shadowed_variables: SET plpgsql.extra_warnings TO 'shadowed_variables'; @@ -5081,8 +5117,41 @@ LINE 3: f1 int; ^ CREATE FUNCTION - - + The below example shows the effects of setting + plpgsql.extra_warnings to + strict_multi_assignment: + +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_warnings is active. +HINT: Make sure the query returns the exact list of columns. +WARNING: number of source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_warnings is active. +HINT: Make sure the query returns the exact list of columns. + + foo +----- + +(1 row) + + + diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index e39f7357bd..8c1a135187 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -4020,6 +4020,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 @@ -4059,9 +4065,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 @@ -4070,7 +4077,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) tcount = 2; else tcount = 1; @@ -4187,19 +4194,28 @@ 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)) { char *errdetail; + int errlevel; + bool use_errhint; + bool force_error; if (estate->func->print_strict_params) errdetail = format_expr_params(estate, expr); else errdetail = NULL; - ereport(ERROR, + force_error = stmt->strict || stmt->mod_stmt; + errlevel = force_error ? ERROR : too_many_rows_level; + use_errhint = !force_error; + + 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("Make sure the query returns a single row, or use LIMIT 1") : 0)); } /* Put the first result row into the target */ exec_move_row(estate, target, tuptab->vals[0], tuptab->tupdesc); @@ -6835,6 +6851,19 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate, int td_natts = tupdesc ? tupdesc->natts : 0; int fnum; int anum; + int strict_multiassignment_level = 0; + + /* + * The extra check strict strict_multi_assignment can be active, + * only when input tupdesc is specified. + */ + if (tupdesc != NULL) + { + if (plpgsql_extra_errors & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT) + strict_multiassignment_level = ERROR; + else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT) + strict_multiassignment_level = WARNING; + } /* Handle RECORD-target case */ if (target->dtype == PLPGSQL_DTYPE_REC) @@ -6913,10 +6942,23 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate, } else { + /* no source for destination column */ value = (Datum) 0; isnull = true; valtype = UNKNOWNOID; valtypmod = -1; + + /* When source value is missing */ + if (strict_multiassignment_level) + ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("number of source and target fields in assignment do not match"), + /* translator: %s represents a name of an extra check */ + errdetail("%s check of %s is active.", + "strict_multi_assignment", + strict_multiassignment_level == ERROR ? "extra_errors" : + "extra_warnings"), + errhint("Make sure the query returns the exact list of columns."))); } /* Cast the new value to the right type, if needed. */ @@ -6930,6 +6972,29 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate, newnulls[fnum] = isnull; } + /* + * When strict_multiassignment extra check is active, then ensure + * there are no unassigned source attributes. + */ + if (strict_multiassignment_level && anum < td_natts) + { + /* skip dropped columns in the source descriptor */ + while (anum < td_natts && + TupleDescAttr(tupdesc, anum)->attisdropped) + anum++; + + if (anum < td_natts) + ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("number of source and target fields in assignment do not match"), + /* translator: %s represents a name of an extra check */ + errdetail("%s check of %s is active.", + "strict_multi_assignment", + strict_multiassignment_level == ERROR ? "extra_errors" : + "extra_warnings"), + errhint("Make sure the query returns the exact list of columns."))); + } + values = newvalues; nulls = newnulls; } @@ -6986,16 +7051,50 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate, } else { + /* no source for destination column */ value = (Datum) 0; isnull = true; valtype = UNKNOWNOID; valtypmod = -1; + + if (strict_multiassignment_level) + ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("number of source and target fields in assignment do not match"), + /* translator: %s represents a name of an extra check */ + errdetail("%s check of %s is active.", + "strict_multi_assignment", + strict_multiassignment_level == ERROR ? "extra_errors" : + "extra_warnings"), + errhint("Make sure the query returns the exact list of columns."))); } exec_assign_value(estate, (PLpgSQL_datum *) var, value, isnull, valtype, valtypmod); } + /* + * When strict_multiassignment extra check is active, ensure there + * are no unassigned source attributes. + */ + if (strict_multiassignment_level && 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 source and target fields in assignment do not match"), + /* translator: %s represents a name of an extra check */ + errdetail("%s check of %s is active.", + "strict_multi_assignment", + strict_multiassignment_level == ERROR ? "extra_errors" : + "extra_warnings"), + errhint("Make sure the query returns the exact list of columns."))); + } + return; } diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 61452d9f7f..7d3647a12d 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 fe617791df..2153ac2f7b 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1137,7 +1137,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 b687fbfddc..2b86c8f7ac 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3113,6 +3113,107 @@ 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: Make sure the query returns a single row, or use LIMIT 1 +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: Make sure the query returns a single row, or use LIMIT 1 +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_warnings is active. +HINT: Make sure the query returns the exact list of columns. +WARNING: number of source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_warnings is active. +HINT: Make sure the query returns the exact list of columns. +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_errors is active. +HINT: Make sure the query returns the exact list of columns. +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; +-- the check is active only when source table is not empty +insert into test_01 values(10,20); +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_errors is active. +HINT: Make sure the query returns the exact list of columns. +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_errors is active. +HINT: Make sure the query returns the exact list of columns. +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 source and target fields in assignment do not match +DETAIL: strict_multi_assignment check of extra_errors is active. +HINT: Make sure the query returns the exact list of columns. +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 e71d072aa9..01239e26be 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -2627,6 +2627,95 @@ 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; + +-- the check is active only when source table is not empty +insert into test_01 values(10,20); + +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 $$