On 2014-08-12 13:23, I wrote:
The compile-time raise parameter checking is a good move.
3 minor points:
- I would suggest to avoid "continue" within a loop so that the code is
simpler to understand, at least for me.
I personally find the code easier to read with the continue.
I've changed the loop slightly. Do you find this more readable than the
way the loop was previously written?
- I would suggest to update the documentation accordingly.
I've incorporated these changes into this version of the patch, with
small changes.
On 2014-08-12 15:09, Fabien COELHO wrote:
> I'm not sure why elog is better than ereport in that case: ISTM that
it is
> an error worth reporting if it ever happens, say there is another syntax
> added later on which is not caught for some reason by the compile-time
> check, so I would not change it.
With elog(ERROR, ..) it's still reported, but the user isn't fooled into
thinking that the error is to be expected, and hopefully we would see a
bug report. If it's impossible to tell the two errors apart, we might
have subtly broken code carried around for who knows how long.
Please let me know what you think about this patch. Thanks for your
work so far.
.marko
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
***************
*** 3403,3408 **** RAISE ;
--- 3403,3411 ----
Inside the format string, <literal>%</literal> is replaced by the
string representation of the next optional argument's value. Write
<literal>%%</literal> to emit a literal <literal>%</literal>.
+ The number of arguments must match the number of <literal>%</>
+ placeholders in the format string, or an error is raised during
+ the compilation of the function.
</para>
<para>
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 2939,2948 **** exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
continue;
}
if (current_param == NULL)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("too few parameters specified for RAISE")));
paramvalue = exec_eval_expr(estate,
(PLpgSQL_expr *) lfirst(current_param),
--- 2939,2947 ----
continue;
}
+ /* should have been checked by the compiler */
if (current_param == NULL)
! elog(ERROR, "unexpected RAISE parameter list length");
paramvalue = exec_eval_expr(estate,
(PLpgSQL_expr *) lfirst(current_param),
***************
*** 2963,2976 **** exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
appendStringInfoChar(&ds, cp[0]);
}
! /*
! * If more parameters were specified than were required to process the
! * format string, throw an error
! */
if (current_param != NULL)
! ereport(ERROR,
! (errcode(ERRCODE_SYNTAX_ERROR),
! errmsg("too many parameters specified for RAISE")));
err_message = ds.data;
/* No pfree(ds.data), the pfree(err_message) does it */
--- 2962,2970 ----
appendStringInfoChar(&ds, cp[0]);
}
! /* should have been checked by the compiler */
if (current_param != NULL)
! elog(ERROR, "unexpected RAISE parameter list length");
err_message = ds.data;
/* No pfree(ds.data), the pfree(err_message) does it */
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
***************
*** 106,111 **** static void check_labels(const char *start_label,
--- 106,112 ----
static PLpgSQL_expr *read_cursor_args(PLpgSQL_var *cursor,
int until, const char *expected);
static List *read_raise_options(void);
+ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%}
***************
*** 1849,1854 **** stmt_raise : K_RAISE
--- 1850,1857 ----
new->options = read_raise_options();
}
+ check_raise_parameters(new);
+
$$ = (PLpgSQL_stmt *)new;
}
;
***************
*** 3768,3773 **** read_raise_options(void)
--- 3771,3810 ----
}
/*
+ * Check that the number of parameter placeholders in the message matches the
+ * number of parameters passed to it, if message was defined.
+ */
+ static void
+ check_raise_parameters(PLpgSQL_stmt_raise *stmt)
+ {
+ char *cp;
+ int expected_nparams = 0;
+
+ if (stmt->message == NULL)
+ return;
+
+ for (cp = stmt->message; *cp; cp++)
+ {
+ if (cp[0] != '%')
+ continue;
+ /* ignore literal % characters */
+ if (cp[1] == '%')
+ cp++;
+ else
+ expected_nparams++;
+ }
+
+ if (expected_nparams < list_length(stmt->params))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("too many parameters specified for RAISE")));
+ if (expected_nparams > list_length(stmt->params))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("too few parameters specified for RAISE")));
+ }
+
+ /*
* Fix up CASE statement
*/
static PLpgSQL_stmt *
*** a/src/test/regress/expected/plpgsql.out
--- b/src/test/regress/expected/plpgsql.out
***************
*** 2446,2463 **** begin
return $1;
end;
$$ language plpgsql;
- select raise_test1(5);
ERROR: too many parameters specified for RAISE
! CONTEXT: PL/pgSQL function raise_test1(integer) line 3 at RAISE
create function raise_test2(int) returns int as $$
begin
raise notice 'This message has too few parameters: %, %, %', $1, $1;
return $1;
end;
$$ language plpgsql;
- select raise_test2(10);
ERROR: too few parameters specified for RAISE
! CONTEXT: PL/pgSQL function raise_test2(integer) line 3 at RAISE
-- Test re-RAISE inside a nested exception block. This case is allowed
-- by Oracle's PL/SQL but was handled differently by PG before 9.1.
CREATE FUNCTION reraise_test() RETURNS void AS $$
--- 2446,2474 ----
return $1;
end;
$$ language plpgsql;
ERROR: too many parameters specified for RAISE
! CONTEXT: compilation of PL/pgSQL function "raise_test1" near line 3
create function raise_test2(int) returns int as $$
begin
raise notice 'This message has too few parameters: %, %, %', $1, $1;
return $1;
end;
$$ language plpgsql;
ERROR: too few parameters specified for RAISE
! CONTEXT: compilation of PL/pgSQL function "raise_test2" near line 3
! create function raise_test3(int) returns int as $$
! begin
! raise notice 'This message has no parameters (despite having %% signs in it)!';
! return $1;
! end;
! $$ language plpgsql;
! select raise_test3(1);
! NOTICE: This message has no parameters (despite having % signs in it)!
! raise_test3
! -------------
! 1
! (1 row)
!
-- Test re-RAISE inside a nested exception block. This case is allowed
-- by Oracle's PL/SQL but was handled differently by PG before 9.1.
CREATE FUNCTION reraise_test() RETURNS void AS $$
*** a/src/test/regress/sql/plpgsql.sql
--- b/src/test/regress/sql/plpgsql.sql
***************
*** 2078,2085 **** begin
end;
$$ language plpgsql;
- select raise_test1(5);
-
create function raise_test2(int) returns int as $$
begin
raise notice 'This message has too few parameters: %, %, %', $1, $1;
--- 2078,2083 ----
***************
*** 2087,2093 **** begin
end;
$$ language plpgsql;
! select raise_test2(10);
-- Test re-RAISE inside a nested exception block. This case is allowed
-- by Oracle's PL/SQL but was handled differently by PG before 9.1.
--- 2085,2098 ----
end;
$$ language plpgsql;
! create function raise_test3(int) returns int as $$
! begin
! raise notice 'This message has no parameters (despite having %% signs in it)!';
! return $1;
! end;
! $$ language plpgsql;
!
! select raise_test3(1);
-- Test re-RAISE inside a nested exception block. This case is allowed
-- by Oracle's PL/SQL but was handled differently by PG before 9.1.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
