plpgsql - additional extra checks

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

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

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.

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

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 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.

fixed doc attached

Regards

Pavel

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

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.c

Marked as Waiting on Author.

I am sending updated code. It reflects Tom's changes - now, the rec is used
as row type too, so the checks must be on two places. With this update is
related one change. When result is empty, then the extra checks doesn't
work - PLpgSQL runtime doesn't pass necessary tupledesc. But usually, when
result is empty, then there are not problems with missing values, because
every value is NULL.

Regards

Pavel

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.c

Marked as Waiting on Author.

I am sending updated code. It reflects Tom's changes - now, the rec is
used as row type too, so the checks must be on two places. With this
update is related one change. When result is empty, then the extra
checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
But usually, when result is empty, then there are not problems with
missing values, because every value is NULL.

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

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.c

Marked as Waiting on Author.

I am sending updated code. It reflects Tom's changes - now, the rec is
used as row type too, so the checks must be on two places. With this
update is related one change. When result is empty, then the extra
checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
But usually, when result is empty, then there are not problems with
missing values, because every value is NULL.

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.

Tomas, thank you for correction.

Regards

Pavel

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.c

Marked as Waiting on Author.

I am sending updated code. It reflects Tom's changes - now, the rec is
used as row type too, so the checks must be on two places. With this
update is related one change. When result is empty, then the extra
checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
But usually, when result is empty, then there are not problems with
missing values, because every value is NULL.

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.

I am sending updated patch with Tomas changes

Regards

Pavel