Hi
2018-03-01 21:14 GMT+01:00 David Steele <da...@pgmasters.net>:
> Hi Pavel,
>
> On 1/7/18 3:31 AM, Pavel Stehule wrote:
> >
> > There, now it's in the correct Waiting for Author state. :)
> >
> > thank you for comments. All should be fixed in attached patch
>
> This patch no longer applies (and the conflicts do not look trivial).
> Can you provide a rebased patch?
>
> $ git apply -3 ../other/plpgsql-extra-check-180107.patch
> error: patch failed: src/pl/plpgsql/src/pl_exec.c:5944
> Falling back to three-way merge...
> Applied patch to 'src/pl/plpgsql/src/pl_exec.c' with conflicts.
> U src/pl/plpgsql/src/pl_exec.c
>
> Marked as Waiting on Author.
>
I am sending updated code. It reflects Tom's changes - now, the rec is used
as row type too, so the checks must be on two places. With this update is
related one change. When result is empty, then the extra checks doesn't
work - PLpgSQL runtime doesn't pass necessary tupledesc. But usually, when
result is empty, then there are not problems with missing values, because
every value is NULL.
Regards
Pavel
>
> Thanks,
> --
> -David
> da...@pgmasters.net
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..1657ec3fb5 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4987,7 +4987,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
@@ -4999,26 +4999,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';
@@ -5034,8 +5070,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 4ff87e0879..6a1c30e769 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3809,6 +3809,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
@@ -3847,9 +3853,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
@@ -3858,7 +3865,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;
@@ -3972,19 +3979,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);
@@ -6602,6 +6616,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)
@@ -6680,10 +6707,19 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
else
{
+ /* there are no data */
value = (Datum) 0;
isnull = true;
valtype = UNKNOWNOID;
valtypmod = -1;
+
+ /* When source value is missing */
+ 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")));
}
/* Cast the new value to the right type, if needed. */
@@ -6697,6 +6733,24 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
newnulls[fnum] = isnull;
}
+ /*
+ * When stric_multiassignment extra check is active, then ensure so there
+ * are not more unassigned source value.
+ */
+ 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")));
+ }
+
values = newvalues;
nulls = newnulls;
}
@@ -6757,12 +6811,38 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
isnull = true;
valtype = UNKNOWNOID;
valtypmod = -1;
+
+ /* When source value is missing */
+ 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 source value.
+ */
+ 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 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 26a7344e9a..c9ad099ab1 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1111,7 +1111,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 722715049c..254580fc41 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 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;
+-- 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 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 f405d157bb..ec958cf3b4 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 $$
