Skip to content

Commit 167075b

Browse files
tvondratvondra
committed
Add strict_multi_assignment and too_many_rows plpgsql checks
Until now shadowed_variables was the only plpgsql check supported by plpgsql.extra_warnings and plpgsql.extra_errors. This patch introduces two new checks - strict_multi_assignment and too_many_rows. Unlike shadowed_variables, these new checks are enforced at run-time. strict_multi_assignment checks that commands allowing multi-assignment (for example SELECT INTO) have the same number of sources and targets. too_many_rows checks that queries with an INTO clause return one row exactly. These checks are aimed at cases that are technically valid and allowed, but are often a sign of a bug. Therefore those checks are expected to be enabled primarily in development and testing environments. Author: Pavel Stehule Reviewed-by: Stephen Frost, Tomas Vondra Discussion: https://www.postgresql.org/message-id/flat/CAFj8pRA2kKRDKpUNwLY0GeG1OqOp+tLS2yQA1V41gzuSz-hCng@mail.gmail.com
1 parent 2d30675 commit 167075b

File tree

6 files changed

+400
-33
lines changed

6 files changed

+400
-33
lines changed

‎doc/src/sgml/plpgsql.sgml

Lines changed: 93 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5034,7 +5034,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
50345034

50355035
</sect2>
50365036
<sect2 id="plpgsql-extra-checks">
5037-
<title>Additional Compile-time Checks</title>
5037+
<title>Additional Compile-time and Run-time Checks</title>
50385038

50395039
<para>
50405040
To aid the user in finding instances of simple but common problems before
@@ -5046,26 +5046,64 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
50465046
so you are advised to test in a separate development environment.
50475047
</para>
50485048

5049-
<para>
5050-
These additional checks are enabled through the configuration variables
5051-
<varname>plpgsql.extra_warnings</varname> for warnings and
5052-
<varname>plpgsql.extra_errors</varname> for errors. Both can be set either to
5053-
a comma-separated list of checks, <literal>"none"</literal> or <literal>"all"</literal>.
5054-
The default is <literal>"none"</literal>. Currently the list of available checks
5055-
includes only one:
5056-
<variablelist>
5057-
<varlistentry>
5058-
<term><varname>shadowed_variables</varname></term>
5059-
<listitem>
5060-
<para>
5061-
Checks if a declaration shadows a previously defined variable.
5062-
</para>
5063-
</listitem>
5064-
</varlistentry>
5065-
</variablelist>
5049+
<para>
5050+
Setting <varname>plpgsql.extra_warnings</varname>, or
5051+
<varname>plpgsql.extra_errors</varname>, as appropriate, to <literal>"all"</literal>
5052+
is encouraged in development and/or testing environments.
5053+
</para>
5054+
5055+
<para>
5056+
These additional checks are enabled through the configuration variables
5057+
<varname>plpgsql.extra_warnings</varname> for warnings and
5058+
<varname>plpgsql.extra_errors</varname> for errors. Both can be set either to
5059+
a comma-separated list of checks, <literal>"none"</literal> or
5060+
<literal>"all"</literal>. The default is <literal>"none"</literal>. Currently
5061+
the list of available checks includes:
5062+
<variablelist>
5063+
<varlistentry>
5064+
<term><varname>shadowed_variables</varname></term>
5065+
<listitem>
5066+
<para>
5067+
Checks if a declaration shadows a previously defined variable.
5068+
</para>
5069+
</listitem>
5070+
</varlistentry>
50665071

5067-
The following example shows the effect of <varname>plpgsql.extra_warnings</varname>
5068-
set to <varname>shadowed_variables</varname>:
5072+
<varlistentry>
5073+
<term><varname>strict_multi_assignment</varname></term>
5074+
<listitem>
5075+
<para>
5076+
Some <application>PL/PgSQL</application> commands allow assigning
5077+
values to more than one variable at a time, such as
5078+
<command>SELECT INTO</command>. Typically, the number of target
5079+
variables and the number of source variables should match, though
5080+
<application>PL/PgSQL</application> will use <literal>NULL</literal>
5081+
for missing values and extra variables are ignored. Enabling this
5082+
check will cause <application>PL/PgSQL</application> to throw a
5083+
<literal>WARNING</literal> or <literal>ERROR</literal> whenever the
5084+
number of target variables and the number of source variables are
5085+
different.
5086+
</para>
5087+
</listitem>
5088+
</varlistentry>
5089+
5090+
<varlistentry>
5091+
<term><varname>too_many_rows</varname></term>
5092+
<listitem>
5093+
<para>
5094+
Enabling this check will cause <application>PL/PgSQL</application> to
5095+
check if a given query returns more than one row when an
5096+
<literal>INTO</literal> clause is used. As an <literal>INTO</literal>
5097+
statement will only ever use one row, having a query return multiple
5098+
rows is generally either inefficient and/or nondeterministic and
5099+
therefore is likely an error.
5100+
</para>
5101+
</listitem>
5102+
</varlistentry>
5103+
</variablelist>
5104+
5105+
The following example shows the effect of <varname>plpgsql.extra_warnings</varname>
5106+
set to <varname>shadowed_variables</varname>:
50695107
<programlisting>
50705108
SET plpgsql.extra_warnings TO 'shadowed_variables';
50715109

@@ -5081,8 +5119,41 @@ LINE 3: f1 int;
50815119
^
50825120
CREATE FUNCTION
50835121
</programlisting>
5084-
</para>
5085-
</sect2>
5122+
The below example shows the effects of setting
5123+
<varname>plpgsql.extra_warnings</varname> to
5124+
<varname>strict_multi_assignment</varname>:
5125+
<programlisting>
5126+
SET plpgsql.extra_warnings TO 'strict_multi_assignment';
5127+
5128+
CREATE OR REPLACE FUNCTION public.foo()
5129+
RETURNS void
5130+
LANGUAGE plpgsql
5131+
AS $$
5132+
DECLARE
5133+
x int;
5134+
y int;
5135+
BEGIN
5136+
SELECT 1 INTO x, y;
5137+
SELECT 1, 2 INTO x, y;
5138+
SELECT 1, 2, 3 INTO x, y;
5139+
END;
5140+
$$;
5141+
5142+
SELECT foo();
5143+
WARNING: number of source and target fields in assignment do not match
5144+
DETAIL: strict_multi_assignment check of extra_warnings is active.
5145+
HINT: Make sure the query returns the exact list of columns.
5146+
WARNING: number of source and target fields in assignment do not match
5147+
DETAIL: strict_multi_assignment check of extra_warnings is active.
5148+
HINT: Make sure the query returns the exact list of columns.
5149+
5150+
foo
5151+
-----
5152+
5153+
(1 row)
5154+
</programlisting>
5155+
</para>
5156+
</sect2>
50865157
</sect1>
50875158

50885159
<!-- **** Porting from Oracle PL/SQL **** -->

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 101 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4020,6 +4020,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
40204020
long tcount;
40214021
int rc;
40224022
PLpgSQL_expr *expr = stmt->sqlstmt;
4023+
int too_many_rows_level = 0;
4024+
4025+
if (plpgsql_extra_errors & PLPGSQL_XCHECK_TOOMANYROWS)
4026+
too_many_rows_level = ERROR;
4027+
else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_TOOMANYROWS)
4028+
too_many_rows_level = WARNING;
40234029

40244030
/*
40254031
* On the first call for this statement generate the plan, and detect
@@ -4059,9 +4065,10 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
40594065

40604066
/*
40614067
* If we have INTO, then we only need one row back ... but if we have INTO
4062-
* STRICT, ask for two rows, so that we can verify the statement returns
4063-
* only one. INSERT/UPDATE/DELETE are always treated strictly. Without
4064-
* INTO, just run the statement to completion (tcount = 0).
4068+
* STRICT or extra check too_many_rows, ask for two rows, so that we can
4069+
* verify the statement returns only one. INSERT/UPDATE/DELETE are always
4070+
* treated strictly. Without INTO, just run the statement to completion
4071+
* (tcount = 0).
40654072
*
40664073
* We could just ask for two rows always when using INTO, but there are
40674074
* some cases where demanding the extra row costs significant time, eg by
@@ -4070,7 +4077,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
40704077
*/
40714078
if (stmt->into)
40724079
{
4073-
if (stmt->strict || stmt->mod_stmt)
4080+
if (stmt->strict || stmt->mod_stmt || too_many_rows_level)
40744081
tcount = 2;
40754082
else
40764083
tcount = 1;
@@ -4187,19 +4194,23 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
41874194
}
41884195
else
41894196
{
4190-
if (n > 1 && (stmt->strict || stmt->mod_stmt))
4197+
if (n > 1 && (stmt->strict || stmt->mod_stmt || too_many_rows_level))
41914198
{
41924199
char *errdetail;
4200+
int errlevel;
41934201

41944202
if (estate->func->print_strict_params)
41954203
errdetail = format_expr_params(estate, expr);
41964204
else
41974205
errdetail = NULL;
41984206

4199-
ereport(ERROR,
4207+
errlevel = (stmt->strict || stmt->mod_stmt) ? ERROR : too_many_rows_level;
4208+
4209+
ereport(errlevel,
42004210
(errcode(ERRCODE_TOO_MANY_ROWS),
42014211
errmsg("query returned more than one row"),
4202-
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0));
4212+
errdetail ? errdetail_internal("parameters: %s", errdetail) : 0,
4213+
errhint("Make sure the query returns a single row, or use LIMIT 1")));
42034214
}
42044215
/* Put the first result row into the target */
42054216
exec_move_row(estate, target, tuptab->vals[0], tuptab->tupdesc);
@@ -6835,6 +6846,19 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
68356846
int td_natts = tupdesc ? tupdesc->natts : 0;
68366847
int fnum;
68376848
int anum;
6849+
int strict_multiassignment_level = 0;
6850+
6851+
/*
6852+
* The extra check strict strict_multi_assignment can be active,
6853+
* only when input tupdesc is specified.
6854+
*/
6855+
if (tupdesc != NULL)
6856+
{
6857+
if (plpgsql_extra_errors & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT)
6858+
strict_multiassignment_level = ERROR;
6859+
else if (plpgsql_extra_warnings & PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT)
6860+
strict_multiassignment_level = WARNING;
6861+
}
68386862

68396863
/* Handle RECORD-target case */
68406864
if (target->dtype == PLPGSQL_DTYPE_REC)
@@ -6913,10 +6937,23 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
69136937
}
69146938
else
69156939
{
6940+
/* no source for destination column */
69166941
value = (Datum) 0;
69176942
isnull = true;
69186943
valtype = UNKNOWNOID;
69196944
valtypmod = -1;
6945+
6946+
/* When source value is missing */
6947+
if (strict_multiassignment_level)
6948+
ereport(strict_multiassignment_level,
6949+
(errcode(ERRCODE_DATATYPE_MISMATCH),
6950+
errmsg("number of source and target fields in assignment do not match"),
6951+
/* translator: %s represents a name of an extra check */
6952+
errdetail("%s check of %s is active.",
6953+
"strict_multi_assignment",
6954+
strict_multiassignment_level == ERROR ? "extra_errors" :
6955+
"extra_warnings"),
6956+
errhint("Make sure the query returns the exact list of columns.")));
69206957
}
69216958

69226959
/* Cast the new value to the right type, if needed. */
@@ -6930,6 +6967,29 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
69306967
newnulls[fnum] = isnull;
69316968
}
69326969

6970+
/*
6971+
* When strict_multiassignment extra check is active, then ensure
6972+
* there are no unassigned source attributes.
6973+
*/
6974+
if (strict_multiassignment_level && anum < td_natts)
6975+
{
6976+
/* skip dropped columns in the source descriptor */
6977+
while (anum < td_natts &&
6978+
TupleDescAttr(tupdesc, anum)->attisdropped)
6979+
anum++;
6980+
6981+
if (anum < td_natts)
6982+
ereport(strict_multiassignment_level,
6983+
(errcode(ERRCODE_DATATYPE_MISMATCH),
6984+
errmsg("number of source and target fields in assignment do not match"),
6985+
/* translator: %s represents a name of an extra check */
6986+
errdetail("%s check of %s is active.",
6987+
"strict_multi_assignment",
6988+
strict_multiassignment_level == ERROR ? "extra_errors" :
6989+
"extra_warnings"),
6990+
errhint("Make sure the query returns the exact list of columns.")));
6991+
}
6992+
69336993
values = newvalues;
69346994
nulls = newnulls;
69356995
}
@@ -6986,16 +7046,50 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate,
69867046
}
69877047
else
69887048
{
7049+
/* no source for destination column */
69897050
value = (Datum) 0;
69907051
isnull = true;
69917052
valtype = UNKNOWNOID;
69927053
valtypmod = -1;
7054+
7055+
if (strict_multiassignment_level)
7056+
ereport(strict_multiassignment_level,
7057+
(errcode(ERRCODE_DATATYPE_MISMATCH),
7058+
errmsg("number of source and target fields in assignment do not match"),
7059+
/* translator: %s represents a name of an extra check */
7060+
errdetail("%s check of %s is active.",
7061+
"strict_multi_assignment",
7062+
strict_multiassignment_level == ERROR ? "extra_errors" :
7063+
"extra_warnings"),
7064+
errhint("Make sure the query returns the exact list of columns.")));
69937065
}
69947066

69957067
exec_assign_value(estate, (PLpgSQL_datum *) var,
69967068
value, isnull, valtype, valtypmod);
69977069
}
69987070

7071+
/*
7072+
* When strict_multiassignment extra check is active, ensure there
7073+
* are no unassigned source attributes.
7074+
*/
7075+
if (strict_multiassignment_level && anum < td_natts)
7076+
{
7077+
while (anum < td_natts &&
7078+
TupleDescAttr(tupdesc, anum)->attisdropped)
7079+
anum++; /* skip dropped column in tuple */
7080+
7081+
if (anum < td_natts)
7082+
ereport(strict_multiassignment_level,
7083+
(errcode(ERRCODE_DATATYPE_MISMATCH),
7084+
errmsg("number of source and target fields in assignment do not match"),
7085+
/* translator: %s represents a name of an extra check */
7086+
errdetail("%s check of %s is active.",
7087+
"strict_multi_assignment",
7088+
strict_multiassignment_level == ERROR ? "extra_errors" :
7089+
"extra_warnings"),
7090+
errhint("Make sure the query returns the exact list of columns.")));
7091+
}
7092+
69997093
return;
70007094
}
70017095

‎src/pl/plpgsql/src/pl_handler.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,10 @@ plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source)
9292

9393
if (pg_strcasecmp(tok, "shadowed_variables") == 0)
9494
extrachecks |= PLPGSQL_XCHECK_SHADOWVAR;
95+
else if (pg_strcasecmp(tok, "too_many_rows") == 0)
96+
extrachecks |= PLPGSQL_XCHECK_TOOMANYROWS;
97+
else if (pg_strcasecmp(tok, "strict_multi_assignment") == 0)
98+
extrachecks |= PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT;
9599
else if (pg_strcasecmp(tok, "all") == 0 || pg_strcasecmp(tok, "none") == 0)
96100
{
97101
GUC_check_errdetail("Key word \"%s\" cannot be combined with other key words.", tok);

‎src/pl/plpgsql/src/plpgsql.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,10 +1135,12 @@ extern bool plpgsql_print_strict_params;
11351135

11361136
extern bool plpgsql_check_asserts;
11371137

1138-
/* extra compile-time checks */
1139-
#define PLPGSQL_XCHECK_NONE 0
1140-
#define PLPGSQL_XCHECK_SHADOWVAR 1
1141-
#define PLPGSQL_XCHECK_ALL ((int) ~0)
1138+
/* extra compile-time and run-time checks */
1139+
#define PLPGSQL_XCHECK_NONE 0
1140+
#define PLPGSQL_XCHECK_SHADOWVAR (1 << 1)
1141+
#define PLPGSQL_XCHECK_TOOMANYROWS (1 << 2)
1142+
#define PLPGSQL_XCHECK_STRICTMULTIASSIGNMENT (1 << 3)
1143+
#define PLPGSQL_XCHECK_ALL ((int) ~0)
11421144

11431145
extern int plpgsql_extra_warnings;
11441146
extern int plpgsql_extra_errors;

0 commit comments

Comments
 (0)