On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> Hi
>
> 2018-03-01 21:14 GMT+01:00 David Steele <da...@pgmasters.net
> <mailto:da...@pgmasters.net>>:
>
> Hi Pavel,
>
> On 1/7/18 3:31 AM, Pavel Stehule wrote:
> >
> > There, now it's in the correct Waiting for Author state. :)
> >
> > thank you for comments. All should be fixed in attached patch
>
> This patch no longer applies (and the conflicts do not look trivial).
> Can you provide a rebased patch?
>
> $ git apply -3 ../other/plpgsql-extra-check-180107.patch
> error: patch failed: src/pl/plpgsql/src/pl_exec.c:5944
> Falling back to three-way merge...
> Applied patch to 'src/pl/plpgsql/src/pl_exec.c' with conflicts.
> U src/pl/plpgsql/src/pl_exec.c
>
> Marked as Waiting on Author.
>
>
> I am sending updated code. It reflects Tom's changes - now, the rec is
> used as row type too, so the checks must be on two places. With this
> update is related one change. When result is empty, then the extra
> checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
> But usually, when result is empty, then there are not problems with
> missing values, because every value is NULL.
>
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
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 1657ec3..ebdd0be 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5027,12 +5027,12 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
<listitem>
<para>
Some <application>PL/PgSQL</application> commands allow assigning values
- to more than one variable at a time, such as SELECT INTO. Typically,
+ to more than one variable at a time, such as <command>SELECT INTO</command>. Typically,
the number of target variables and the number of source variables should
- match, though <application>PL/PgSQL</application> will use NULL for
+ match, though <application>PL/PgSQL</application> will use <literal>NULL</literal> for
missing values and extra variables are ignored. Enabling this check
- will cause <application>PL/PgSQL</application> to throw a WARNING or
- ERROR whenever the number of target variables and the number of source
+ will cause <application>PL/PgSQL</application> to throw a <literal>WARNING</literal> or
+ <literal>ERROR</literal> whenever the number of target variables and the number of source
variables are different.
</para>
</listitem>
@@ -5044,7 +5044,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
<para>
Enabling this check will cause <application>PL/PgSQL</application> to
check if a given query returns more than one row when an
- <literal>INTO</literal> clause is used. As an INTO statement will only
+ <literal>INTO</literal> clause is used. As an <literal>INTO</literal> statement will only
ever use one row, having a query return multiple rows is generally
either inefficient and/or nondeterministic and therefore is likely an
error.
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 44e8edb..0d9815b 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3867,7 +3867,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
*/
if (stmt->into)
{
- if (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING)
+ if (stmt->strict || stmt->mod_stmt || too_many_rows_level)
tcount = 2;
else
tcount = 1;
@@ -3981,7 +3981,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
}
else
{
- if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level >= WARNING))
+ if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level))
{
char *errdetail;
int errlevel;
@@ -3997,7 +3997,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
ereport(errlevel,
(errcode(ERRCODE_TOO_MANY_ROWS),
- errmsg("query returned more than one row"),
+ errmsg("SELECT INTO query returned more than one row"),
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0,
use_errhint ? errhint("too_many_rows check of extra_%s is active.",
too_many_rows_level == ERROR ? "errors" : "warnings") : 0));
@@ -6709,17 +6709,17 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
else
{
- /* there are no data */
+ /* no source for destination column */
value = (Datum) 0;
isnull = true;
valtype = UNKNOWNOID;
valtypmod = -1;
/* When source value is missing */
- if (strict_multiassignment_level >= WARNING)
+ if (strict_multiassignment_level)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ errmsg("Number of source and target fields in assignment does not match."),
errhint("strict_multi_assignement check of extra_%s is active.",
strict_multiassignment_level == ERROR ? "errors" : "warnings")));
}
@@ -6736,19 +6736,20 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
/*
- * When stric_multiassignment extra check is active, then ensure so there
- * are not more unassigned source value.
+ * When strict_multiassignment extra check is active, then ensure
+ * there are no unassigned source attributes.
*/
- if (strict_multiassignment_level >= WARNING && anum < td_natts)
+ if (strict_multiassignment_level && anum < td_natts)
{
+ /* skip dropped columns in the source descriptor */
while (anum < td_natts &&
TupleDescAttr(tupdesc, anum)->attisdropped)
- anum++; /* skip dropped column in tuple */
+ anum++;
if (anum < td_natts)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ errmsg("Number of source and target fields in assignment does not match."),
errhint("strict_multi_assignement check of extra_%s is active.",
strict_multiassignment_level == ERROR ? "errors" : "warnings")));
}
@@ -6809,16 +6810,16 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
else
{
+ /* no source for destination column */
value = (Datum) 0;
isnull = true;
valtype = UNKNOWNOID;
valtypmod = -1;
- /* When source value is missing */
- if (strict_multiassignment_level >= WARNING)
+ if (strict_multiassignment_level)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ errmsg("Number of source and target fields in assignment does not match."),
errhint("strict_multi_assignement check of extra_%s is active.",
strict_multiassignment_level == ERROR ? "errors" : "warnings")));
}
@@ -6828,10 +6829,10 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
}
/*
- * When stric_multiassignment extra check is active, then ensure so there
- * are not more unassigned source value.
+ * When strict_multiassignment extra check is active, ensure there
+ * are no unassigned source attributes.
*/
- if (strict_multiassignment_level >= WARNING && anum < td_natts)
+ if (strict_multiassignment_level && anum < td_natts)
{
while (anum < td_natts &&
TupleDescAttr(tupdesc, anum)->attisdropped)
@@ -6840,7 +6841,7 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
if (anum < td_natts)
ereport(strict_multiassignment_level,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- errmsg("Number of evaluated fields does not match expected."),
+ errmsg("Number of source and target fields in assignment does not match."),
errhint("strict_multi_assignement check of extra_%s is active.",
strict_multiassignment_level == ERROR ? "errors" : "warnings")));
}
