plpgsql - additional extra checks
Hi
I am starting new thread for this patch (related to merging some ideas from
plpgsql2 project thread).
Here is simple patch with two new extra_warnings, extra_errors checks
1. strict_multi_assignment - checks the number of target variables and
source values.
2. too_many_rows - checks if returned rows is more than one
The extra checks are designed to help identify some possible errors or
issues. It is not way how to change a design, behave and features of
plpgsql language.
Now, the extra checks are three fields only - it is easy maintainable now,
and I don't see a necessity to implement some another management for extra
checks.
Regards
Pavel
Attachments:
plpgsql_extra_runtime_checks-01.patchtext/x-patch; charset=US-ASCII; name=plpgsql_extra_runtime_checks-01.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d3272e1..f81dea0 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4847,7 +4847,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
@@ -4859,6 +4859,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</> to <literal>all</> is a
+ good idea in developer or test environments.
+ </para>
+
<para>
These additional checks are enabled through the configuration variables
<varname>plpgsql.extra_warnings</> for warnings and
@@ -4875,6 +4880,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</> 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>
+
+ <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>
The following example shows the effect of <varname>plpgsql.extra_warnings</>
@@ -4894,6 +4923,34 @@ LINE 3: f1 int;
^
CREATE FUNCTION
</programlisting>
+
+ The another example shows the effect of <varname>plpgsql.extra_warnings</>
+ set to <varname>strict_multi_assignment</>:
+<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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+ foo
+-----
+
+(1 row)
+</programlisting>
</para>
</sect2>
</sect1>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 192cbcf..880c1b8 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3616,6 +3616,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;
+ }
/*
* On the first call for this statement generate the plan, and detect
@@ -3666,7 +3684,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;
@@ -3786,7 +3804,7 @@ 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_check))
{
char *errdetail;
@@ -3795,7 +3813,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
else
errdetail = NULL;
- ereport(ERROR,
+ ereport(too_many_rows_level == WARNING && !stmt->strict ? WARNING : ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more than one row"),
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0));
@@ -6009,12 +6027,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;
+ }
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 (!tupdesc->attrs[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)));
+ }
+ }
+
anum = 0;
for (fnum = 0; fnum < row->nfields; fnum++)
{
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index ca8c9cb..85acf96 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -89,6 +89,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 3421eed..623af81 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1025,7 +1025,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 79513e4..b09e83a 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3422,6 +3422,54 @@ 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
+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
+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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+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 attributies (1) does not match expected attributies (2)
+CONTEXT: PL/pgSQL function inline_code_block line 6 at SQL statement
+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 877d3ad..aa47e93 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2840,6 +2840,57 @@ 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
+$$;
+
+reset plpgsql.extra_errors;
+reset plpgsql.extra_warnings;
+
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
On Wed, Jan 11, 2017 at 2:54 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:
1. strict_multi_assignment - checks the number of target variables and
source values.
I've proposed this before (maybe around a year ago), except the checks were
done at parse time, rather than runtime. I much prefer that approach. If
I recall correctly, the patch was considered to be good, but not good
enough since it didn't handle all contexts (perhaps FOR loop were missing,
or something; I forget).
2. too_many_rows - checks if returned rows is more than one
I've also proposed this, and it was rejected because it was a runtime
check, and some people don't like runtime checks.
.m
2017-01-11 15:08 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
On Wed, Jan 11, 2017 at 2:54 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:1. strict_multi_assignment - checks the number of target variables and
source values.I've proposed this before (maybe around a year ago), except the checks
were done at parse time, rather than runtime. I much prefer that
approach. If I recall correctly, the patch was considered to be good, but
not good enough since it didn't handle all contexts (perhaps FOR loop were
missing, or something; I forget).
Please, can me send a link? Compile time check is better - but I am not
sure how much increase a complexity of patch. There should not be necessary
data in compile time available. You need a rewriten query - and it is not
available in compile time. More in compile time you should not to check
dynamic SQL.
2. too_many_rows - checks if returned rows is more than one
I've also proposed this, and it was rejected because it was a runtime
check, and some people don't like runtime checks.
This runtime check is well controlled, and should be simply enabled, simply
disabled - more it is almost impossible process this check in compile time.
Regards
Pavel
Show quoted text
.m
On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.
Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
FWIW, I'd also be in favor of a NOMULTI option to INTO, but I don't see
any way to do something like that with var := blah FROM.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-01-13 2:46 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
yes, it is possible.
I am not sure - how to document it. This pattern is undocumented and it is
side effect of our parser.
If somebody use var := (SELECT xxx FROM ..)
what is correct syntax, then exactly only one row is required.
But the extra check is ok, if we technically allows this syntax.
Regards
Pavel
Show quoted text
?
AIUI that's one of the beefs the plpgsql2 project has.
FWIW, I'd also be in favor of a NOMULTI option to INTO, but I don't see
any way to do something like that with var := blah FROM.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
2017-01-13 2:46 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com>:
On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
declare x int;
begin
x := i from generate_series(1,1) g(i);
raise notice 'x=%', x;
end;
$$;
NOTICE: x=1
DO
postgres=# do $$
declare x int;
begin
x := i from generate_series(1,2) g(i);
raise notice 'x=%', x;
end;
$$;
ERROR: query "SELECT i from generate_series(1,2) g(i)" returned more than
one row
CONTEXT: PL/pgSQL function inline_code_block line 4 at assignment
so extra check is not required in this case
?
AIUI that's one of the beefs the plpgsql2 project has.
uff - I hope so plpgsql2 will carry some less scary - against the clean
syntax
x := (select .. )
you save 8 chars. And now the SELECT doesn't look like SELECT - the
statement was broken. This feature is just side effect of plpgsql quick (in
old time little bit poor) design. It is not allowed in PL/SQL and it is not
allowed by SQL/PSM.
FWIW, I'd also be in favor of a NOMULTI option to INTO, but I don't see
any way to do something like that with var := blah FROM.
This is proposed as check for current living code, where you should not to
modify source code.
We can speak about introduce new keyword or new syntax - but it should be
different thread.
Show quoted text
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't think
anyone should.
.m
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.
This patch still applies cleanly and compiles at cccbdde.
--
-David
david@pgmasters.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.
This submission has been moved to CF 2017-07.
--
-David
david@pgmasters.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 08 Apr 2017, at 15:46, David Steele <david@pgmasters.net> wrote:
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.This submission has been moved to CF 2017-07.
This patch was automatically marked as “Waiting for author” since it needs to
be updated with the macro changes in 2cd70845240087da205695baedab6412342d1dbe
to compile. Changing to using TupleDescAttr(); makes it compile again. Can
you submit an updated version with that fix Pavel?
Stephen, you signed up to review this patch in the previous Commitfest, do you
still intend to work on this?
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-13 1:42 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:
On 08 Apr 2017, at 15:46, David Steele <david@pgmasters.net> wrote:
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.This submission has been moved to CF 2017-07.
This patch was automatically marked as “Waiting for author” since it needs
to
be updated with the macro changes in 2cd70845240087da205695baedab64
12342d1dbe
to compile. Changing to using TupleDescAttr(); makes it compile again.
Can
you submit an updated version with that fix Pavel?
I am sending fixed patch
Regards
Pavel
Show quoted text
Stephen, you signed up to review this patch in the previous Commitfest, do
you
still intend to work on this?cheers ./daniel
Attachments:
plpgsql-extra-check-170913.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-170913.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6dc438a152..7de0b8005a 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4862,7 +4862,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
@@ -4874,6 +4874,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</> to <literal>all</> is a
+ good idea in developer or test environments.
+ </para>
+
<para>
These additional checks are enabled through the configuration variables
<varname>plpgsql.extra_warnings</> for warnings and
@@ -4890,6 +4895,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</> 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>
+
+ <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>
The following example shows the effect of <varname>plpgsql.extra_warnings</>
@@ -4909,6 +4938,34 @@ LINE 3: f1 int;
^
CREATE FUNCTION
</programlisting>
+
+ The another example shows the effect of <varname>plpgsql.extra_warnings</>
+ set to <varname>strict_multi_assignment</>:
+<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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+ foo
+-----
+
+(1 row)
+</programlisting>
</para>
</sect2>
</sect1>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 9716697259..c14fdc0233 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3623,6 +3623,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;
+ }
/*
* On the first call for this statement generate the plan, and detect
@@ -3672,7 +3690,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;
@@ -3792,7 +3810,7 @@ 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_check))
{
char *errdetail;
@@ -3801,7 +3819,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
else
errdetail = NULL;
- ereport(ERROR,
+ ereport(too_many_rows_level == WARNING && !stmt->strict ? WARNING : ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more than one row"),
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0));
@@ -6027,12 +6045,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;
+ }
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)));
+ }
+ }
+
anum = 0;
for (fnum = 0; fnum < row->nfields; fnum++)
{
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 1ebb7a7b5e..dceb710703 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 2b19948562..18bff2a27e 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1024,7 +1024,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 7d3e9225bb..17770c1cdc 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3422,6 +3422,54 @@ 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
+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
+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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+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 attributies (1) does not match expected attributies (2)
+CONTEXT: PL/pgSQL function inline_code_block line 6 at SQL statement
+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 6c9399696b..8d0b24ea93 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2840,6 +2840,57 @@ 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
+$$;
+
+reset plpgsql.extra_errors;
+reset plpgsql.extra_warnings;
+
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
On Wed, Sep 13, 2017 at 12:51 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
2017-09-13 1:42 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:
On 08 Apr 2017, at 15:46, David Steele <david@pgmasters.net> wrote:
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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.Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.This submission has been moved to CF 2017-07.
This patch was automatically marked as “Waiting for author” since it needs
to
be updated with the macro changes in
2cd70845240087da205695baedab6412342d1dbe
to compile. Changing to using TupleDescAttr(); makes it compile again.
Can
you submit an updated version with that fix Pavel?I am sending fixed patch
+ <para>
+ The setting <varname>plpgsql.extra_warnings</> to <literal>all</> is a
+ good idea in developer or test environments.
+ </para>
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.
--
Michael
Hi
2017-11-30 3:44 GMT+01:00 Michael Paquier <michael.paquier@gmail.com>:
On Wed, Sep 13, 2017 at 12:51 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:2017-09-13 1:42 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:
On 08 Apr 2017, at 15:46, David Steele <david@pgmasters.net> wrote:
On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby <
Jim.Nasby@bluetreble.com
<mailto:Jim.Nasby@bluetreble.com>> wrote:
On 1/11/17 5:54 AM, Pavel Stehule wrote:
+ <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 thiscase
the assignment
+ is not deterministic usually - and it can be signalsome
issues in design.
Shouldn't this also apply to
var := blah FROM some_table WHERE ...;
?
AIUI that's one of the beefs the plpgsql2 project has.
No, not at all. That syntax is undocumented and only works because
PL/PgSQL is a hack internally. We don't use it, and frankly I don't
think anyone should.This submission has been moved to CF 2017-07.
This patch was automatically marked as “Waiting for author” since it
needs
to
be updated with the macro changes in
2cd70845240087da205695baedab6412342d1dbe
to compile. Changing to using TupleDescAttr(); makes it compile again.
Can
you submit an updated version with that fix Pavel?I am sending fixed patch
+ <para> + The setting <varname>plpgsql.extra_warnings</> to <literal>all</> is a + good idea in developer or test environments. + </para> 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
Regards
Pavel
Show quoted text
--
Michael
Attachments:
plpgsql-extra-check-171130.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-171130.patchDownload
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
@@ -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,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>
+
<para>
These additional checks are enabled through the configuration variables
<varname>plpgsql.extra_warnings</varname> for warnings and
@@ -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>
+
+ <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>
The following example shows the effect of <varname>plpgsql.extra_warnings</varname>
@@ -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>
+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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+ foo
+-----
+
+(1 row)
</programlisting>
</para>
</sect2>
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;
+ }
/*
* On the first call for this statement generate the plan, and detect
@@ -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;
@@ -3798,7 +3816,7 @@ 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_check))
{
char *errdetail;
@@ -3807,7 +3825,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
else
errdetail = NULL;
- ereport(ERROR,
+ ereport(too_many_rows_level == WARNING && !stmt->strict ? WARNING : ERROR,
(errcode(ERRCODE_TOO_MANY_ROWS),
errmsg("query returned more than one row"),
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0));
@@ -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;
+ }
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)));
+ }
+ }
+
anum = 0;
for (fnum = 0; fnum < row->nfields; fnum++)
{
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 1ebb7a7b5e..dceb710703 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 2b19948562..18bff2a27e 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1024,7 +1024,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 d6e5bc3353..72776c2e2f 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3422,6 +3422,54 @@ 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
+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
+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 attributies (1) does not match expected attributies (2)
+WARNING: Number of evaluated attributies (3) does not match expected attributies (2)
+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 attributies (1) does not match expected attributies (2)
+CONTEXT: PL/pgSQL function inline_code_block line 6 at SQL statement
+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 1c355132b7..189750316c 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2840,6 +2840,57 @@ 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
+$$;
+
+reset plpgsql.extra_errors;
+reset plpgsql.extra_warnings;
+
-- test scrollable cursor support
create function sc_test() returns setof integer as $$
Greetings Pavel,
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
2017-11-30 3:44 GMT+01:00 Michael Paquier <michael.paquier@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. :)
Thanks!
Stephen
Hi
2018-01-07 0:59 GMT+01:00 Stephen Frost <sfrost@snowman.net>:
Greetings Pavel,
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
2017-11-30 3:44 GMT+01:00 Michael Paquier <michael.paquier@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
Show quoted text
Thanks!
Stephen
Attachments:
plpgsql-extra-check-180107.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180107.patchDownload
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 $$
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.
Thanks,
--
-David
david@pgmasters.net
Hi
2018-03-01 21:14 GMT+01:00 David Steele <david@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.cMarked 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
Show quoted text
Thanks,
--
-David
david@pgmasters.net
Attachments:
plpgsql-extra-check-180302.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180302.patchDownload
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 $$
On 03/02/2018 10:30 PM, Pavel Stehule wrote:
Hi
2018-03-01 21:14 GMT+01:00 David Steele <david@pgmasters.net
<mailto:david@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.cMarked 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.
I've looked at this patch today, and in general it seems in fairly good
shape - I don't really see any major issues in it that would mean it
can't be promoted to RFC soon.
A couple of comments though:
1) I think the docs are OK, but there are a couple of keywords that
should be wrapped in <literal> or <command> tags, otherwise the
formatting will be incorrect.
I've done that in the attached patch, as it's easier than listing which
keywords/where etc. I haven't wrapped the lines, though, to make it
easier to see the difference in meld or similar tools.
2) The does does a bunch of checks of log level, in the form
if (too_many_rows_level >= WARNING)
which is perhaps a bit too verbose, because the default value of that
variable is 0. So
if (too_many_rows_level)
would be enough, and it makes the checks a bit shorter. Again, this is
done in the attached patch.
3) There is a couple of typos in the comments, like "stric_" instead of
"strict_" and so on. Again, fixed in the patch, along with slightly
rewording a bunch of comments like
/* no source for destination column */
instead of
/* there are no data */
and so on.
4) I have also reworded the text of the two checks. Firstly, I've replaced
query returned more than one row
with
SELECT INTO query returned more than one row
which I think provides additional useful context to the user.
I've also replaced
Number of evaluated fields does not match expected.
with
Number of source and target fields in assignment does not match.
because the original text seems a bit cumbersome to me. It might be
useful to also include the expected/actual number of fields, to provide
a bit more context. That's valuable particularly for WARNING messages,
which do not include information about line numbers (or even function
name). So anything that helps to locate the query (of possibly many in
that function) is valuable.
Stephen: I see you're listed as reviewer on this patch - do you see an
issue blocking this patch from getting RFC? I see you did a review in
January, but Pavel seems to have resolved the issues you identified.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
plpgsql-extra-checks-fixes.patchtext/x-patch; name=plpgsql-extra-checks-fixes.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 1657ec3..ebdd0be 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5027,12 +5027,12 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
<listitem>
<para>
Some <application>PL/PgSQL</application> commands allow assigning values
- to more than one variable at a time, such as SELECT INTO. Typically,
+ 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 NULL for
+ 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 WARNING or
- ERROR whenever the number of target variables and the number of source
+ 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>
@@ -5044,7 +5044,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
<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
+ <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.
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 44e8edb..0d9815b 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3867,7 +3867,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
*/
if (stmt->into)
{
- if (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING)
+ if (stmt->strict || stmt->mod_stmt || too_many_rows_level)
tcount = 2;
else
tcount = 1;
@@ -3981,7 +3981,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
}
else
{
- if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING))
+ if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level))
{
char *errdetail;
int errlevel;
@@ -3997,7 +3997,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
ereport(errlevel,
(errcode(ERRCODE_TOO_MANY_ROWS),
- errmsg("query returned more than one row"),
+ errmsg("SELECT INTO query returned more than one row"),
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));
@@ -6709,17 +6709,17 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
else
{
- /* there are no data */
+ /* no source for destination column */
value = (Datum) 0;
isnull = true;
valtype = UNKNOWNOID;
valtypmod = -1;
/* When source value is missing */
- if (strict_multiassignment_level >= WARNING)
+ if (strict_multiassignment_level)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ 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")));
}
@@ -6736,19 +6736,20 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
/*
- * When stric_multiassignment extra check is active, then ensure so there
- * are not more unassigned source value.
+ * When strict_multiassignment extra check is active, then ensure
+ * there are no unassigned source attributes.
*/
- if (strict_multiassignment_level >= WARNING && anum < td_natts)
+ if (strict_multiassignment_level && anum < td_natts)
{
+ /* skip dropped columns in the source descriptor */
while (anum < td_natts &&
TupleDescAttr(tupdesc, anum)->attisdropped)
- anum++; /* skip dropped column in tuple */
+ anum++;
if (anum < td_natts)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ 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")));
}
@@ -6809,16 +6810,16 @@ 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 >= WARNING)
+ if (strict_multiassignment_level)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ 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")));
}
@@ -6828,10 +6829,10 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
/*
- * When stric_multiassignment extra check is active, then ensure so there
- * are not more unassigned source value.
+ * When strict_multiassignment extra check is active, ensure there
+ * are no unassigned source attributes.
*/
- if (strict_multiassignment_level >= WARNING && anum < td_natts)
+ if (strict_multiassignment_level && anum < td_natts)
{
while (anum < td_natts &&
TupleDescAttr(tupdesc, anum)->attisdropped)
@@ -6840,7 +6841,7 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
if (anum < td_natts)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ 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")));
}
2018-03-04 2:46 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
On 03/02/2018 10:30 PM, Pavel Stehule wrote:
Hi
2018-03-01 21:14 GMT+01:00 David Steele <david@pgmasters.net
<mailto:david@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.cMarked 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.I've looked at this patch today, and in general it seems in fairly good
shape - I don't really see any major issues in it that would mean it
can't be promoted to RFC soon.A couple of comments though:
1) I think the docs are OK, but there are a couple of keywords that
should be wrapped in <literal> or <command> tags, otherwise the
formatting will be incorrect.I've done that in the attached patch, as it's easier than listing which
keywords/where etc. I haven't wrapped the lines, though, to make it
easier to see the difference in meld or similar tools.2) The does does a bunch of checks of log level, in the form
if (too_many_rows_level >= WARNING)
which is perhaps a bit too verbose, because the default value of that
variable is 0. Soif (too_many_rows_level)
would be enough, and it makes the checks a bit shorter. Again, this is
done in the attached patch.3) There is a couple of typos in the comments, like "stric_" instead of
"strict_" and so on. Again, fixed in the patch, along with slightly
rewording a bunch of comments like/* no source for destination column */
instead of
/* there are no data */
and so on.
4) I have also reworded the text of the two checks. Firstly, I've replaced
query returned more than one row
with
SELECT INTO query returned more than one row
which I think provides additional useful context to the user.
I've also replaced
Number of evaluated fields does not match expected.
with
Number of source and target fields in assignment does not match.
because the original text seems a bit cumbersome to me. It might be
useful to also include the expected/actual number of fields, to provide
a bit more context. That's valuable particularly for WARNING messages,
which do not include information about line numbers (or even function
name). So anything that helps to locate the query (of possibly many in
that function) is valuable.
Tomas, thank you for correction.
Regards
Pavel
Show quoted text
Stephen: I see you're listed as reviewer on this patch - do you see an
issue blocking this patch from getting RFC? I see you did a review in
January, but Pavel seems to have resolved the issues you identified.regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2018-03-04 13:37 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
2018-03-04 2:46 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
On 03/02/2018 10:30 PM, Pavel Stehule wrote:
Hi
2018-03-01 21:14 GMT+01:00 David Steele <david@pgmasters.net
<mailto:david@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.cMarked 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.I've looked at this patch today, and in general it seems in fairly good
shape - I don't really see any major issues in it that would mean it
can't be promoted to RFC soon.A couple of comments though:
1) I think the docs are OK, but there are a couple of keywords that
should be wrapped in <literal> or <command> tags, otherwise the
formatting will be incorrect.I've done that in the attached patch, as it's easier than listing which
keywords/where etc. I haven't wrapped the lines, though, to make it
easier to see the difference in meld or similar tools.2) The does does a bunch of checks of log level, in the form
if (too_many_rows_level >= WARNING)
which is perhaps a bit too verbose, because the default value of that
variable is 0. Soif (too_many_rows_level)
would be enough, and it makes the checks a bit shorter. Again, this is
done in the attached patch.3) There is a couple of typos in the comments, like "stric_" instead of
"strict_" and so on. Again, fixed in the patch, along with slightly
rewording a bunch of comments like/* no source for destination column */
instead of
/* there are no data */
and so on.
4) I have also reworded the text of the two checks. Firstly, I've replaced
query returned more than one row
with
SELECT INTO query returned more than one row
which I think provides additional useful context to the user.
I've also replaced
Number of evaluated fields does not match expected.
with
Number of source and target fields in assignment does not match.
because the original text seems a bit cumbersome to me. It might be
useful to also include the expected/actual number of fields, to provide
a bit more context. That's valuable particularly for WARNING messages,
which do not include information about line numbers (or even function
name). So anything that helps to locate the query (of possibly many in
that function) is valuable.
I am sending updated patch with Tomas changes
Regards
Pavel
Show quoted text
Tomas, thank you for correction.
Regards
Pavel
Stephen: I see you're listed as reviewer on this patch - do you see an
issue blocking this patch from getting RFC? I see you did a review in
January, but Pavel seems to have resolved the issues you identified.regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
plpgsql-extra-check-180304.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180304.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..ebdd0be7ef 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 <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';
@@ -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 489484f184..bd0a16aa9b 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3806,6 +3806,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
@@ -3844,9 +3850,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
@@ -3855,7 +3862,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;
@@ -3969,19 +3976,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))
{
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));
+ errmsg("SELECT INTO query returned more than one row"),
+ 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);
@@ -6599,6 +6613,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)
@@ -6677,10 +6704,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. */
@@ -6694,6 +6730,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;
}
@@ -6750,16 +6805,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 dd59036de0..c0df4e3214 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1112,7 +1112,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..e3fead6777 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2777,7 +2777,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
create or replace function footest() returns void as $$
declare x record;
@@ -2850,7 +2850,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
create or replace function footest() returns void as $$
declare x record;
@@ -2914,7 +2914,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
DETAIL: parameters: p1 = '2', p3 = 'foo'
CONTEXT: PL/pgSQL function footest() line 8 at SQL statement
create or replace function footest() returns void as $$
@@ -2925,7 +2925,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
CONTEXT: PL/pgSQL function footest() line 5 at SQL statement
create or replace function footest() returns void as $$
declare x record;
@@ -2972,7 +2972,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
CONTEXT: PL/pgSQL function footest() line 10 at SQL statement
reset plpgsql.print_strict_params;
create or replace function footest() returns void as $$
@@ -2988,7 +2988,7 @@ begin
raise notice 'x.f1 = %, x.f2 = %', x.f1, x.f2;
end$$ language plpgsql;
select footest();
-ERROR: query returned more than one row
+ERROR: SELECT INTO query returned more than one row
DETAIL: parameters: p1 = '2', p3 = 'foo'
CONTEXT: PL/pgSQL function footest() line 10 at SQL statement
-- test warnings and errors
@@ -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: SELECT INTO 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: SELECT INTO 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 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 $$
On 03/04/2018 07:07 PM, Pavel Stehule wrote:
...
I am sending updated patch with Tomas changes
Seems 2cf8c7aa48 broke this patch, as it tweaked a number of regression
tests. Other than that, I think the patch is pretty much ready.
One minor detail is that this bit from exec_stmt_execsql doesn't seem
particularly readable:
errlevel = stmt->strict || stmt->mod_stmt ? ERROR : too_many_rows_level;
use_errhint = !(stmt->strict || stmt->mod_stmt);
I had to think for a while if "||" takes precedence over "?", so I'd
suggest adding some () around the first condition. But that makes it
pretty much just (!use_errhint) so perhaps something like
bool force_error;
force_error = (stmt->strict || stmt->mod_stmt);
errlevel = force_error ? ERROR : too_many_rows_level;
use_errhint = !force_error;
would be better?
Actually, now that I'm looking at this part of the code again, I see the
change from
errmsg("query returned more than one row"),
to
errmsg("SELECT INTO query returned more than one row"),
is probably bogus, because this also deals with stmt->mod_stmt, not just
strict SELECT INTO queries. So let's just revert to the old wording.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2018-03-16 2:46 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
On 03/04/2018 07:07 PM, Pavel Stehule wrote:
...
I am sending updated patch with Tomas changes
Seems 2cf8c7aa48 broke this patch, as it tweaked a number of regression
tests. Other than that, I think the patch is pretty much ready.One minor detail is that this bit from exec_stmt_execsql doesn't seem
particularly readable:errlevel = stmt->strict || stmt->mod_stmt ? ERROR : too_many_rows_level;
use_errhint = !(stmt->strict || stmt->mod_stmt);I had to think for a while if "||" takes precedence over "?", so I'd
suggest adding some () around the first condition. But that makes it
pretty much just (!use_errhint) so perhaps something likebool force_error;
force_error = (stmt->strict || stmt->mod_stmt);
errlevel = force_error ? ERROR : too_many_rows_level;
use_errhint = !force_error;would be better?
good idea
Actually, now that I'm looking at this part of the code again, I see the
change fromerrmsg("query returned more than one row"),
to
errmsg("SELECT INTO query returned more than one row"),
is probably bogus, because this also deals with stmt->mod_stmt, not just
strict SELECT INTO queries. So let's just revert to the old wording.
fixed
Show quoted text
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
plpgsql-extra-check-180316.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180316.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6c25116538..c95f168250 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 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 68da7cf669..d390e4b840 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 $$
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."
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
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@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
Show quoted text
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
Attachments:
plpgsql-extra-check-180320.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180320.patchDownload
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 $$
On 03/20/2018 05:36 AM, Pavel Stehule wrote:
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
<mailto:tomas.vondra@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
OK, thanks. PFA I've marked it as ready for committer.
I think the patch is solid code-wise, but I'm sure it might benefit e.g.
from a native speaker checking the wording of comments and messages.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Pavel and Tomas,
On Tue, March 20, 2018 12:36 am, Pavel Stehule wrote:
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@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."
I believe the correct wording should be:
"Number of source and target fields in assignment do not match."
ecause comparing one number to the other means "the number A and B _do_
not match", not "the number A does not match number B".
Also there is an inconsistent quoting here:
+ <para>
+ Setting <varname>plpgsql.extra_warnings</varname>, or
+ <varname>plpgsql.extra_errors</varname>, as appropriate, to
<literal>all</literal>
no quotes for 'all'.
+ 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>.
quotes here around '"all"'. I think it should be one or the other in both
cases.
Also:
+ Currently
+ the list of available checks includes only one:
but then it lists more than one check?
Best wishes,
Tels
2018-03-20 13:56 GMT+01:00 Tels <nospam-abuse@bloodgate.com>:
Hello Pavel and Tomas,
On Tue, March 20, 2018 12:36 am, Pavel Stehule wrote:
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@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."I believe the correct wording should be:
"Number of source and target fields in assignment do not match."
ecause comparing one number to the other means "the number A and B _do_
not match", not "the number A does not match number B".Also there is an inconsistent quoting here:
+ <para> + Setting <varname>plpgsql.extra_warnings</varname>, or + <varname>plpgsql.extra_errors</varname>, as appropriate, to <literal>all</literal>no quotes for 'all'.
+ 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>.quotes here around '"all"'. I think it should be one or the other in both
cases.Also:
+ Currently + the list of available checks includes only one:but then it lists more than one check?
all mentioned issues should be fixed
Thank you for your time :)
Regards
Pavel
Show quoted text
Best wishes,
Tels
Attachments:
plpgsql-extra-check-180320-2.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180320-2.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7ed926fd51..807e4a66a1 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:
+ <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 do not match.
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: Number of source and target fields in assignment do 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..ad22680086 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 do 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 do 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 do 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 do 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..40d827d20d 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 do not match.
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: Number of source and target fields in assignment do 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 do 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 do 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 do 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 do 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 $$
On 03/20/2018 01:35 PM, Tomas Vondra wrote:
On 03/20/2018 05:36 AM, Pavel Stehule wrote:
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
<mailto:tomas.vondra@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
OK, thanks. PFA I've marked it as ready for committer.
Stephen, what are your thoughts about this patch? I remember discussing
it with you at pgcon, but I don't recall what exactly your complaints
were. Do you see any problems with the current version of the patch?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 07/03/2018 03:45 PM, Tomas Vondra wrote:
On 03/20/2018 01:35 PM, Tomas Vondra wrote:
On 03/20/2018 05:36 AM, Pavel Stehule wrote:
2018-03-19 21:47 GMT+01:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
<mailto:tomas.vondra@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
OK, thanks. PFA I've marked it as ready for committer.
Stephen, what are your thoughts about this patch? I remember discussing
it with you at pgcon, but I don't recall what exactly your complaints
were. Do you see any problems with the current version of the patch?
(tumbleweed)
This is marked as RFC for quite a bit of time now. Barring objections, I
plan to commit sometime this later this week, after going through the
patch once more just to be sure.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
+ 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));
Please write this in a way that results in less translatable messages,
and don't build setting names at runtime. Concretely I suggest this:
errhint(too_many_rows_level == ERROR ?
gettext_noop("%s check of extra_errors is active.") :
gettext_noop("%s check of extra_warnings is active."),
"too_many_rows");
This way,
1. only two messages need translation, not one per type of warning/error
2. the translator doesn't need to scratch their head to figure out what
the second %s is
3. they don't have to worry about introducing a typo in the string
"too_many_rows" or the strings "extra_errors", "extra_warnings".
(Laugh all you want. It's a real problem).
If you can add a /* translator: */ comment to indicate what the first %s
is, all the better. I'm just not sure *where* it needs to go. I'm not
100% sure the gettext_noop() is really needed either.
+ ereport(strict_multiassignment_level, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("Number of source and target fields in assignment do not match."),
Please, no uppercase in errmsg(), and no ending period.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-07-09 15:44:36 -0400, Alvaro Herrera wrote:
+ 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));Please write this in a way that results in less translatable messages,
and don't build setting names at runtime. Concretely I suggest this:errhint(too_many_rows_level == ERROR ?
gettext_noop("%s check of extra_errors is active.") :
gettext_noop("%s check of extra_warnings is active."),
"too_many_rows");
Why not put extra_errors/extra_warnings into a variable as well?
Greetings,
Andres Freund
On 2018-Jul-09, Andres Freund wrote:
On 2018-07-09 15:44:36 -0400, Alvaro Herrera wrote:
+ 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));Please write this in a way that results in less translatable messages,
and don't build setting names at runtime. Concretely I suggest this:errhint(too_many_rows_level == ERROR ?
gettext_noop("%s check of extra_errors is active.") :
gettext_noop("%s check of extra_warnings is active."),
"too_many_rows");Why not put extra_errors/extra_warnings into a variable as well?
Yeah, what he said. (Then you *really* need a /* translator: */ comment
to explain.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2018-07-09 21:44 GMT+02:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
+ 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));
Please write this in a way that results in less translatable messages,
and don't build setting names at runtime. Concretely I suggest this:errhint(too_many_rows_level == ERROR ?
gettext_noop("%s check of extra_errors is active.") :
gettext_noop("%s check of extra_warnings is active."),
"too_many_rows");This way,
1. only two messages need translation, not one per type of warning/error
2. the translator doesn't need to scratch their head to figure out what
the second %s is
3. they don't have to worry about introducing a typo in the string
"too_many_rows" or the strings "extra_errors", "extra_warnings".
(Laugh all you want. It's a real problem).If you can add a /* translator: */ comment to indicate what the first %s
is, all the better. I'm just not sure *where* it needs to go. I'm not
100% sure the gettext_noop() is really needed either.+ ereport(strict_
multiassignment_level,
+
(errcode(ERRCODE_DATATYPE_MISMATCH),
+
errmsg("Number of source and target fields in assignment do not match."),
Please, no uppercase in errmsg(), and no ending period.
Thank you for notes. Tomorrow morning I'll spend few hours in train and
I'll send updated patch
Regards
Pavel
Show quoted text
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi
2018-07-09 21:44 GMT+02:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
+ 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));
Please write this in a way that results in less translatable messages,
and don't build setting names at runtime. Concretely I suggest this:errhint(too_many_rows_level == ERROR ?
gettext_noop("%s check of extra_errors is active.") :
gettext_noop("%s check of extra_warnings is active."),
"too_many_rows");This way,
1. only two messages need translation, not one per type of warning/error
2. the translator doesn't need to scratch their head to figure out what
the second %s is
3. they don't have to worry about introducing a typo in the string
"too_many_rows" or the strings "extra_errors", "extra_warnings".
(Laugh all you want. It's a real problem).If you can add a /* translator: */ comment to indicate what the first %s
is, all the better. I'm just not sure *where* it needs to go. I'm not
100% sure the gettext_noop() is really needed either.+ ereport(strict_
multiassignment_level,
+
(errcode(ERRCODE_DATATYPE_MISMATCH),
+
errmsg("Number of source and target fields in assignment do not match."),
Please, no uppercase in errmsg(), and no ending period.
should be fixed now.
Regards
Pavel
Show quoted text
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
plpgsql-extra-check-180712-1.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180712-1.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 5b2aac618e..58cb5d4d8b 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5050,7 +5050,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
@@ -5062,26 +5062,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:
+ <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';
@@ -5097,8 +5133,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 do not match.
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: Number of source and target fields in assignment do 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 e39f7357bd..f9491ad448 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,31 @@ 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_level == ERROR ?
+ /* translator: %s represent a name of extra check */
+ gettext_noop("%s check of extra_errors is active.") :
+ gettext_noop("%s check of extra_warnings is active."),
+ "too_many_rows") : 0));
}
/* Put the first result row into the target */
exec_move_row(estate, target, tuptab->vals[0], tuptab->tupdesc);
@@ -6835,6 +6854,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 +6945,22 @@ 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"),
+ errhint(strict_multiassignment_level == ERROR ?
+ /* translator: %s represent a name of extra check */
+ gettext_noop("%s check of extra_errors is active.") :
+ gettext_noop("%s check of extra_warnings is active."),
+ "strict_multi_assignement")));
}
/* Cast the new value to the right type, if needed. */
@@ -6930,6 +6974,28 @@ 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"),
+ errhint(strict_multiassignment_level == ERROR ?
+ /* translator: %s represent a name of extra check */
+ gettext_noop("%s check of extra_errors is active.") :
+ gettext_noop("%s check of extra_warnings is active."),
+ "strict_multi_assignement")));
+ }
+
values = newvalues;
nulls = newnulls;
}
@@ -6986,16 +7052,48 @@ 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"),
+ errhint(strict_multiassignment_level == ERROR ?
+ /* translator: %s represent a name of extra check */
+ gettext_noop("%s check of extra_errors is active.") :
+ gettext_noop("%s check of extra_warnings is active."),
+ "strict_multi_assignement")));
}
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"),
+ errhint(strict_multiassignment_level == ERROR ?
+ /* translator: %s represent a name of extra check */
+ gettext_noop("%s check of extra_errors is active.") :
+ gettext_noop("%s check of extra_warnings is active."),
+ "strict_multi_assignement")));
+ }
+
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..6d306cd769 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 do not match
+HINT: strict_multi_assignement check of extra_warnings is active.
+WARNING: number of source and target fields in assignment do 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 do 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 do 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 do 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 do 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 $$
Hi,
I've been looking at the version submitted on Thursday, planning to
commit it, but I think it needs a wee bit more work on the error messages.
1) The example in sgml docs does not work, because it's missing a
semicolon after the CREATE FUNCTION. And the output was not updated
after tweaking the messages, so it still has uppercase+comma.
2) The
/* translator: %s represent a name of extra check */
comment should be
/* translator: %s represents a name of an extra check */
3) Both Andres and Alvaro suggested to pass extra_errors/extra_warnings
as a variable too, turning the errhint into
errhint("%s check of %s is active.",
"too_many_rows",
(too_many_rows_level == ERROR) ? "extra_errors" :
"extra_checks");
or something like that. Any particular reason not to do that?
Sorry for the bikeshedding, but I'd like my first commit not to be
immediately torn apart by wolves ;-)
4) I think the errhint() is used incorrectly. The message should be
about how to fix the issue, but messages like
HINT: strict_multi_assignement check of extra_warnings is active.
clearly does not help in this regard. This information should be either
in errdetail() is deemed useful, or left out entirely. And the errhint()
should say something like:
errdetail("Make sure the query returns a single row, or use LIMIT 1.")
and
errdetail("Make sure the query returns the exact list of columns.")
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
2018-07-15 1:38 GMT+02:00 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
Hi,
I've been looking at the version submitted on Thursday, planning to
commit it, but I think it needs a wee bit more work on the error messages.1) The example in sgml docs does not work, because it's missing a
semicolon after the CREATE FUNCTION. And the output was not updated
after tweaking the messages, so it still has uppercase+comma.
fixed
2) The
/* translator: %s represent a name of extra check */
comment should be
/* translator: %s represents a name of an extra check */
3) Both Andres and Alvaro suggested to pass extra_errors/extra_warnings
as a variable too, turning the errhint intoerrhint("%s check of %s is active.",
"too_many_rows",
(too_many_rows_level == ERROR) ? "extra_errors" :
"extra_checks");or something like that. Any particular reason not to do that?
errdetail was used on first place already.
Now, I skip detail in this first case, and elsewhere I move this info to
detail, and hint has text that you proposed.
Sorry for the bikeshedding, but I'd like my first commit not to be
immediately torn apart by wolves ;-)
no problem
4) I think the errhint() is used incorrectly. The message should be
about how to fix the issue, but messages likeHINT: strict_multi_assignement check of extra_warnings is active.
clearly does not help in this regard. This information should be either
in errdetail() is deemed useful, or left out entirely. And the errhint()
should say something like:errdetail("Make sure the query returns a single row, or use LIMIT 1.")
and
errdetail("Make sure the query returns the exact list of columns.")
should be fixed too
Regards
Pavel
Show quoted text
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
plpgsql-extra-check-180719-1.patchtext/x-patch; charset=US-ASCII; name=plpgsql-extra-check-180719-1.patchDownload
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 '$$
</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
@@ -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.
</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:
+ <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';
@@ -5081,8 +5117,41 @@ 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 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)
+</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 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 $$
On 07/19/2018 02:50 PM, Pavel Stehule wrote:
2018-07-15 1:38 GMT+02:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
<mailto:tomas.vondra@2ndquadrant.com>>:Hi,
I've been looking at the version submitted on Thursday, planning to
commit it, but I think it needs a wee bit more work on the error
messages.1) The example in sgml docs does not work, because it's missing a
semicolon after the CREATE FUNCTION. And the output was not updated
after tweaking the messages, so it still has uppercase+comma.fixed
2) The
/* translator: %s represent a name of extra check */
comment should be
/* translator: %s represents a name of an extra check */
3) Both Andres and Alvaro suggested to pass extra_errors/extra_warnings
as a variable too, turning the errhint intoerrhint("%s check of %s is active.",
"too_many_rows",
(too_many_rows_level == ERROR) ? "extra_errors" :
"extra_checks");or something like that. Any particular reason not to do that?
errdetail was used on first place already.
Now, I skip detail in this first case, and elsewhere I move this info to
detail, and hint has text that you proposed.Sorry for the bikeshedding, but I'd like my first commit not to be
immediately torn apart by wolves ;-)no problem
4) I think the errhint() is used incorrectly. The message should be
about how to fix the issue, but messages likeHINT: strict_multi_assignement check of extra_warnings is active.
clearly does not help in this regard. This information should be either
in errdetail() is deemed useful, or left out entirely. And the errhint()
should say something like:errdetail("Make sure the query returns a single row, or use LIMIT 1.")
and
errdetail("Make sure the query returns the exact list of columns.")
should be fixed too
Seems OK to me. I'll go over the patch once more tomorrow and I plan to
get it committed.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 07/22/2018 10:24 PM, Tomas Vondra wrote:
On 07/19/2018 02:50 PM, Pavel Stehule wrote:
2018-07-15 1:38 GMT+02:00 Tomas Vondra <tomas.vondra@2ndquadrant.com
<mailto:tomas.vondra@2ndquadrant.com>>:Hi,
I've been looking at the version submitted on Thursday, planning to
commit it, but I think it needs a wee bit more work on the error
messages.1) The example in sgml docs does not work, because it's missing a
semicolon after the CREATE FUNCTION. And the output was not updated
after tweaking the messages, so it still has uppercase+comma.fixed
2) The
/* translator: %s represent a name of extra check */
comment should be
/* translator: %s represents a name of an extra check */
3) Both Andres and Alvaro suggested to pass extra_errors/extra_warnings
as a variable too, turning the errhint intoerrhint("%s check of %s is active.",
"too_many_rows",
(too_many_rows_level == ERROR) ? "extra_errors" :
"extra_checks");or something like that. Any particular reason not to do that?
errdetail was used on first place already.
Now, I skip detail in this first case, and elsewhere I move this info to
detail, and hint has text that you proposed.Sorry for the bikeshedding, but I'd like my first commit not to be
immediately torn apart by wolves ;-)no problem
4) I think the errhint() is used incorrectly. The message should be
about how to fix the issue, but messages likeHINT: strict_multi_assignement check of extra_warnings is active.
clearly does not help in this regard. This information should be either
in errdetail() is deemed useful, or left out entirely. And the errhint()
should say something like:errdetail("Make sure the query returns a single row, or use LIMIT 1.")
and
errdetail("Make sure the query returns the exact list of columns.")
should be fixed too
Seems OK to me. I'll go over the patch once more tomorrow and I plan to
get it committed.
Committed, with some minor changes:
1) The too_many_rows check in exec_stmt_execsql included the errhint
conditionally, depending on force_error variable
force_error = stmt->strict || stmt->mod_stmt;
use_errhint = !force_error;
That is, the hint was not included when force_error=true, which does not
seem quite necessary. Based on off-list discussion with Pavel this was
unnecessary, so the hint is now included always.
2) The comment in plpgsql.h still mentioned "compile-time" checks only,
but the new checks are run-time checks. So tweaked accordingly.
3) A couple of minor formatting / code style changes.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Committed, with some minor changes:
1) The too_many_rows check in exec_stmt_execsql included the errhint
conditionally, depending on force_error variableforce_error = stmt->strict || stmt->mod_stmt;
use_errhint = !force_error;That is, the hint was not included when force_error=true, which does not
seem quite necessary. Based on off-list discussion with Pavel this was
unnecessary, so the hint is now included always.2) The comment in plpgsql.h still mentioned "compile-time" checks only,
but the new checks are run-time checks. So tweaked accordingly.3) A couple of minor formatting / code style changes.
Many thanks
Pavel
Show quoted text
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services