2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.von...@2ndquadrant.com>:
> Hi,
>
> I'm looking at the updated patch (plpgsql-extra-check-180316.patch), and
> this time it applies and builds OK. The one thing I noticed is that the
> documentation still uses the old wording for strict_multi_assignement:
>
> 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.
>
> This was reworded to "Number of source and target fields in assignment
> does not match."
>
fixed
Regards
Pavel
>
> Otherwise it seems fine to me, and I'm tempted to mark it RFC once the
> docs get fixed. Stephen, any objections?
>
> regards
>
> --
> Tomas Vondra http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6c25116538..2027e35063 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5003,7 +5003,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
@@ -5015,26 +5015,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 <command>SELECT INTO</command>. Typically,
+ the number of target variables and the number of source variables should
+ match, though <application>PL/PgSQL</application> will use <literal>NULL</literal> for
+ missing values and extra variables are ignored. Enabling this check
+ will cause <application>PL/PgSQL</application> to throw a <literal>WARNING</literal> or
+ <literal>ERROR</literal> 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 <literal>INTO</literal> 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';
@@ -5050,8 +5086,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 source and target fields in assignment does not match.
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: Number of source and target fields in assignment does not match.
+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 38ea7a091f..2d3dc66726 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3929,6 +3929,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
@@ -3967,9 +3973,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
@@ -3978,7 +3985,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;
@@ -4092,19 +4099,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("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);
@@ -6722,6 +6738,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)
@@ -6800,10 +6829,19 @@ 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 does not match."),
+ errhint("strict_multi_assignement check of extra_%s is active.",
+ strict_multiassignment_level == ERROR ? "errors" : "warnings")));
}
/* Cast the new value to the right type, if needed. */
@@ -6817,6 +6855,25 @@ 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 does not match."),
+ errhint("strict_multi_assignement check of extra_%s is active.",
+ strict_multiassignment_level == ERROR ? "errors" : "warnings")));
+ }
+
values = newvalues;
nulls = newnulls;
}
@@ -6873,16 +6930,42 @@ 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 does not match."),
+ 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 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 does not match."),
+ 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 f38ef04077..08eb530d09 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 f7619a63f9..3d015cd571 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1124,7 +1124,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..246a586b82 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3113,6 +3113,101 @@ 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 source and target fields in assignment does not match.
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: Number of source and target fields in assignment does not match.
+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 source and target fields in assignment does not match.
+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;
+-- 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 does not match.
+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 source and target fields in assignment does not match.
+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 source and target fields in assignment does not match.
+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 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 $$
