Thread: [HACKERS] 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
Attachment
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 oneI'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
.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)
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
?
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.
--
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
> 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
> 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 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
Regards
Pavel
Stephen, you signed up to review this patch in the previous Commitfest, do you
still intend to work on this?
cheers ./daniel
Attachment
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>:
+ <para>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
+ 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
--
Michael
Attachment
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
Attachment
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
Thanks!
Stephen
Attachment
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
Thanks,
--
-David
david@pgmasters.net
Attachment
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
Attachment
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
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.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
Tomas, thank you for correction.RegardsPavel
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
Attachment
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 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?
good idea
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.
fixed
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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
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
Attachment
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
Best wishes,
Tels
Attachment
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
--
Á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
--
Álvaro Herrera https://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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 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?
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 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.")
should be fixed too
Regards
Pavel
regards
--
Tomas Vondra http://www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
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 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?
>
>
> 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 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.")
>
>
> 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 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?
>>
>>
>> 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 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.")
>>
>>
>> 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 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.
Many thanks
Pavel
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services