Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

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
>

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

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

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

On 07/19/2018 02:50 PM, Pavel Stehule wrote:
> 
> 
> 2018-07-15 1:38 GMT+02:00 Tomas Vondra  >:
> 
> 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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-07-15 1:38 GMT+02:00 Tomas Vondra :

> 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.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d6688e13f4..0bf76beb55 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5034,7 +5034,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5046,26 +5046,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to "all"
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5081,8 +5117,41 @@ LINE 3: f1 

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

Hi

2018-07-09 21:44 GMT+02:00 Alvaro Herrera :

> > + 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 Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 5b2aac618e..58cb5d4d8b 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5050,7 +5050,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5062,26 +5062,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to "all"
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of 

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-07-09 21:44 GMT+02:00 Alvaro Herrera :

> > + 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 Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] plpgsql - additional extra checks

[Andres Freund]

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

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-20 13:56 GMT+01:00 Tels :

> 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 :
> >
> >> 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:
>
> +   
> +Setting plpgsql.extra_warnings, or
> +plpgsql.extra_errors, as appropriate, to
> all
>
> no quotes for 'all'.
>
> +is encouraged in development and/or testing environments.
> +   
> +
> +   
> +These additional checks are enabled through the configuration
> variables
> +plpgsql.extra_warnings for warnings and
> +plpgsql.extra_errors for errors. Both can be set
> either to
> +a comma-separated list of checks, "none" or
> +"all".
>
> 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
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7ed926fd51..807e4a66a1 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5003,7 +5003,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5015,26 +5015,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to "all"
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5050,8 +5086,38 @@ LINE 3: f1 

Re: [HACKERS] plpgsql - additional extra checks

[Tels]

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

+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to
all

no quotes for 'all'.

+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set
either to
+a comma-separated list of checks, "none" or
+"all".

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

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

On 03/20/2018 05:36 AM, Pavel Stehule wrote:
> 
> 
> 2018-03-19 21:47 GMT+01:00 Tomas Vondra  >:
> 
> 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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-19 21:47 GMT+01:00 Tomas Vondra :

> Hi,
>
> I'm looking at the updated patch (plpgsql-extra-check-180316.patch), and
> this time it applies and builds OK. The one thing I noticed is that the
> documentation still uses the old wording for strict_multi_assignement:
>
> WARNING:  Number of evaluated fields does not match expected.
> HINT:  strict_multi_assignement check of extra_warnings is active.
> WARNING:  Number of evaluated fields does not match expected.
> HINT:  strict_multi_assignement check of extra_warnings is active.
>
> This was reworded to "Number of source and target fields in assignment
> does not match."
>

fixed

Regards

Pavel


>
> Otherwise it seems fine to me, and I'm tempted to mark it RFC once the
> docs get fixed. Stephen, any objections?
>
> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6c25116538..2027e35063 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5003,7 +5003,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5015,26 +5015,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to all
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes only one:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5050,8 +5086,38 @@ LINE 3: f1 int;
 ^
 CREATE FUNCTION
 
- 
- 
+The below example shows the effects of setting
+plpgsql.extra_warnings to
+strict_multi_assignment:
+
+SET plpgsql.extra_warnings TO 'strict_multi_assignment';
+
+CREATE OR REPLACE FUNCTION public.foo()
+ RETURNS void
+ LANGUAGE plpgsql
+AS $$
+DECLARE
+  x int;
+  y int;
+BEGIN
+  SELECT 1 INTO x, y;
+  SELECT 1, 2 INTO x, y;
+  SELECT 1, 2, 3 INTO x, y;
+END;
+$$
+
+SELECT foo();
+WARNING:  Number of source and target fields in assignment does not match.
+HINT:  strict_multi_assignement check of extra_warnings is active.
+WARNING:  Number of source and target fields in assignment does not match.
+HINT:  strict_multi_assignement check of extra_warnings is active.
+ foo 
+-
+ 
+(1 row)
+
+   
+  
  
 
   
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 38ea7a091f..2d3dc66726 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3929,6 +3929,12 @@ 

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-16 2:46 GMT+01:00 Tomas Vondra :

> 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
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 6c25116538..c95f168250 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5003,7 +5003,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -5015,26 +5015,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to all
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes only one:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5050,8 +5086,38 @@ LINE 3: f1 int;
 ^
 CREATE FUNCTION
 
- 
- 
+The below example shows the effects of setting
+plpgsql.extra_warnings to
+strict_multi_assignment:
+
+SET plpgsql.extra_warnings TO 'strict_multi_assignment';
+
+CREATE OR REPLACE FUNCTION public.foo()
+ RETURNS void
+ LANGUAGE plpgsql
+AS $$
+DECLARE
+  x int;
+  y int;
+BEGIN
+  SELECT 1 INTO x, y;
+  SELECT 1, 2 INTO x, y;
+  SELECT 1, 2, 3 INTO x, y;
+END;
+$$
+
+SELECT foo();
+WARNING:  Number 

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

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

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-04 13:37 GMT+01:00 Pavel Stehule :

>
>
> 2018-03-04 2:46 GMT+01:00 Tomas Vondra :
>
>> On 03/02/2018 10:30 PM, Pavel Stehule wrote:
>> > Hi
>> >
>> > 2018-03-01 21:14 GMT+01:00 David Steele > > >:
>> >
>> > 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  or  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.
>
> 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
>>
>
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..ebdd0be7ef 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4987,7 +4987,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -4999,26 +4999,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

2018-03-04 2:46 GMT+01:00 Tomas Vondra :

> On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> > Hi
> >
> > 2018-03-01 21:14 GMT+01:00 David Steele  > >:
> >
> > 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  or  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
>

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> Hi
> 
> 2018-03-01 21:14 GMT+01:00 David Steele  >:
> 
> 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  or  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
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 1657ec3..ebdd0be 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5027,12 +5027,12 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
   

 Some PL/PgSQL commands allow assigning values
-to more than one variable at a time, such as SELECT INTO.  Typically,
+to more than one variable at a time, such as SELECT INTO.  Typically,
 the number of target variables and the number of source variables should
-match, though PL/PgSQL will use NULL for
+match, though PL/PgSQL will use NULL for
 missing values and extra variables are ignored.  Enabling this check
-will cause PL/PgSQL to throw a WARNING or
-ERROR whenever the number of target variables and the number of source
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
 variables are different.

   
@@ -5044,7 +5044,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$

 Enabling this check will cause PL/PgSQL to
 check if a given query returns more than one row when an
-INTO clause is used.  As an INTO statement will only
+INTO 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 

Re: Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

Hi

2018-03-01 21:14 GMT+01:00 David Steele :

> Hi Pavel,
>
> On 1/7/18 3:31 AM, Pavel Stehule wrote:
> >
> > There, now it's in the correct Waiting for Author state. :)
> >
> > thank you for comments. All should be fixed in attached patch
>
> This patch no longer applies (and the conflicts do not look trivial).
> Can you provide a rebased patch?
>
> $ git apply -3 ../other/plpgsql-extra-check-180107.patch
> error: patch failed: src/pl/plpgsql/src/pl_exec.c:5944
> Falling back to three-way merge...
> Applied patch to 'src/pl/plpgsql/src/pl_exec.c' with conflicts.
> U src/pl/plpgsql/src/pl_exec.c
>
> Marked as Waiting on Author.
>

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

Regards

Pavel



>
> Thanks,
> --
> -David
> da...@pgmasters.net
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index c1e3c6a19d..1657ec3fb5 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -4987,7 +4987,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 
   
   
-   Additional Compile-time Checks
+   Additional Compile-time and Run-time Checks
 

 To aid the user in finding instances of simple but common problems before
@@ -4999,26 +4999,62 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
 so you are advised to test in a separate development environment.

 
- 
-  These additional checks are enabled through the configuration variables
-  plpgsql.extra_warnings for warnings and
-  plpgsql.extra_errors for errors. Both can be set either to
-  a comma-separated list of checks, "none" or "all".
-  The default is "none". Currently the list of available checks
-  includes only one:
-  
-   
-shadowed_variables
-
- 
-  Checks if a declaration shadows a previously defined variable.
- 
-
-   
-  
+   
+Setting plpgsql.extra_warnings, or
+plpgsql.extra_errors, as appropriate, to all
+is encouraged in development and/or testing environments.
+   
+
+   
+These additional checks are enabled through the configuration variables
+plpgsql.extra_warnings for warnings and
+plpgsql.extra_errors for errors. Both can be set either to
+a comma-separated list of checks, "none" or
+"all". The default is "none". Currently
+the list of available checks includes only one:
+
+ 
+  shadowed_variables
+  
+   
+Checks if a declaration shadows a previously defined variable.
+   
+  
+ 
 
-  The following example shows the effect of plpgsql.extra_warnings
-  set to shadowed_variables:
+ 
+  strict_multi_assignment
+  
+   
+Some PL/PgSQL 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 PL/PgSQL will use NULL for
+missing values and extra variables are ignored.  Enabling this check
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
+variables are different.
+   
+  
+ 
+
+ 
+  too_many_rows
+  
+   
+Enabling this check will cause PL/PgSQL to
+check if a given query returns more than one row when an
+INTO 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.
+   
+  
+ 
+
+
+The following example shows the effect of plpgsql.extra_warnings
+set to shadowed_variables:
 
 SET plpgsql.extra_warnings TO 'shadowed_variables';
 
@@ -5034,8 +5070,38 @@ LINE 3: f1 int;
 ^
 CREATE FUNCTION
 
- 
- 
+The below example shows the effects of setting
+plpgsql.extra_warnings to
+strict_multi_assignment:
+
+SET plpgsql.extra_warnings TO 'strict_multi_assignment';
+
+CREATE OR REPLACE FUNCTION public.foo()
+ RETURNS void
+ LANGUAGE plpgsql
+AS $$
+DECLARE
+  x int;
+  y int;
+BEGIN
+  SELECT 1 INTO x, y;
+  SELECT 1, 2 INTO x, y;
+  SELECT 1, 2, 3 INTO x, y;
+END;
+$$
+
+SELECT foo();
+WARNING:  Number of evaluated fields does not match expected.
+HINT:  strict_multi_assignement check of extra_warnings is active.
+WARNING:  Number of evaluated fields does not match expected.
+HINT:  strict_multi_assignement check of extra_warnings is active.
+ foo 
+-
+ 
+(1 row)
+
+   
+  
  
 
   
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 

Re: Re: [HACKERS] plpgsql - additional extra checks

[David Steele]

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
da...@pgmasters.net

Re: [HACKERS] plpgsql - additional extra checks

[Pavel Stehule]

Hi

2018-01-07 0:59 GMT+01:00 Stephen Frost :

> Greetings Pavel,
>
> * Pavel Stehule (pavel.steh...@gmail.com) wrote:
> > 2017-11-30 3:44 GMT+01:00 Michael Paquier :
> > > 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.
> 
>
> +   
> +The setting plpgsql.extra_warnings to
> all is a
> +good idea in developer or test environments.
> +   
>
> Better language for this would be:
>
> Setting plpgsql.extra_warnings, or
> plpgsql.extra_errors, as appropriate, to
> all is encouraged in development and/or testing
> environments.
>
> @@ -4979,6 +4984,30 @@ a_output := a_output || $$ if v_$$ ||
> referrer_keys.kind || $$ like '$$
>   
>  
> 
> +
> +   
> +strict_multi_assignment
> +
> + 
> +  Some PL/PgSQL commands allows to assign
> a values to
> +  more than one variable. The number of target variables should not be
> +  equal to number of source values. Missing values are replaced by
> NULL
> +  value, spare values are ignored. More times this situation signalize
> +  some error.
> + 
> +
> +   
>
> Better language:
>
> Some PL/PgSQL 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 PL/PgSQL will use NULL for
> missing values and extra variables are ignored.  Enabling this check
> will cause PL/PgSQL to throw a WARNING or
> ERROR whenever the number of target variables and the number of source
> variables are different.
>
> +   
> +too_many_rows
> +
> + 
> +  When result is assigned to a variable by INTO
> 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.
> + 
> +
> +   
>
>
> Better language:
>
> Enabling this check will cause PL/PgSQL to
> check if a given query returns more than one row when an
> INTO 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
> +
> +
> +  The another example shows the effect of plpgsql.extra_
> warnings
> +  set to strict_multi_assignment:
> +
>
> Better language:
>
> The below example shows the effects of setting
> plpgsql.extra_warnings to
> strict_multi_assignment:
>
> 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,
> longtcount;
> int rc;
> PLpgSQL_expr *expr = stmt->sqlstmt;
> +   booltoo_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 

Re: [HACKERS] plpgsql - additional extra checks

[Stephen Frost]

Greetings Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> 2017-11-30 3:44 GMT+01:00 Michael Paquier :
> > 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.

 
+   
+The setting plpgsql.extra_warnings to 
all is a 
+good idea in developer or test environments.
+   

Better language for this would be:

Setting plpgsql.extra_warnings, or
plpgsql.extra_errors, as appropriate, to
all is encouraged in development and/or testing
environments.

@@ -4979,6 +4984,30 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind 
|| $$ like '$$
  
 

+
+   
+strict_multi_assignment
+
+ 
+  Some PL/PgSQL commands allows to assign a 
values to
+  more than one variable. The number of target variables should not be
+  equal to number of source values. Missing values are replaced by NULL
+  value, spare values are ignored. More times this situation signalize
+  some error.
+ 
+
+   

Better language:

Some PL/PgSQL 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 PL/PgSQL will use NULL for
missing values and extra variables are ignored.  Enabling this check
will cause PL/PgSQL to throw a WARNING or
ERROR whenever the number of target variables and the number of source
variables are different.

+   
+too_many_rows
+
+ 
+  When result is assigned to a variable by INTO 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.
+ 
+
+   
   

Better language:

Enabling this check will cause PL/PgSQL to
check if a given query returns more than one row when an
INTO 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
+
+
+  The another example shows the effect of 
plpgsql.extra_warnings
+  set to strict_multi_assignment:
+

Better language:

The below example shows the effects of setting
plpgsql.extra_warnings to
strict_multi_assignment:

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,
longtcount;
int rc;
PLpgSQL_expr *expr = stmt->sqlstmt;
+   booltoo_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

Re: [HACKERS] plpgsql - additional extra checks

[Michael Paquier]

On Wed, Sep 13, 2017 at 12:51 PM, Pavel Stehule  wrote:
>
>
> 2017-09-13 1:42 GMT+02:00 Daniel Gustafsson :
>>
>> > On 08 Apr 2017, at 15:46, David Steele  wrote:
>> >
>> >> On 1/13/17 6:55 AM, Marko Tiikkaja wrote:
>> >>> On Fri, Jan 13, 2017 at 2:46 AM, Jim Nasby > >>> > wrote:
>> >>>
>> >>>On 1/11/17 5:54 AM, Pavel Stehule wrote:
>> >>>
>> >>>+too_many_rows
>> >>>+
>> >>>+ 
>> >>>+  When result is assigned to a variable by
>> >>>INTO 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 plpgsql.extra_warnings to all is a
+good idea in developer or test environments.
+   
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