Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hello Marko, I've changed the loop slightly. Do you find this more readable than the way the loop was previously written? It is 50% better:-) It is no big deal, but I still fail to find the remaining continue as usefull in this case. If you remove the continue line and invert the condition, it works exactly the same, so it is just one useless instruction within that loop. From a logical point of view the loop is looking for '%' and then check whether the next char is '%' or not, so the straightforward code helps my understanding as it does exactly that, and the continue is just an hindrance to comprehension. Note that I would buy it if it helped avoid indenting further a significant portion of complex code, but this is not the case here. [doc] I've incorporated these changes into this version of the patch, with small changes. Ok. 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. Ok. In that case, it would make sense to keep distinct wordings of both exceptions in the execution code, so that they also can be set apart, i.e. keep the too many/few somewhere in the error? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
On 09/02/2014 11:52 AM, Fabien COELHO wrote: I've changed the loop slightly. Do you find this more readable than the way the loop was previously written? It is 50% better:-) It is no big deal, but I still fail to find the remaining continue as usefull in this case. If you remove the continue line and invert the condition, it works exactly the same, so it is just one useless instruction within that loop. From a logical point of view the loop is looking for '%' and then check whether the next char is '%' or not, so the straightforward code helps my understanding as it does exactly that, and the continue is just an hindrance to comprehension. Note that I would buy it if it helped avoid indenting further a significant portion of complex code, but this is not the case here. FWIW, I agree. [doc] I've incorporated these changes into this version of the patch, with small changes. Ok. 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. Ok. In that case, it would make sense to keep distinct wordings of both exceptions in the execution code, so that they also can be set apart, i.e. keep the too many/few somewhere in the error? Well, you can do set log_error_verbosity='verbose' if you run into that. I think this patch has been thoroughly reviewed now. Committed, thanks! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hi, On 2014-09-02 15:04, Heikki Linnakangas wrote: I think this patch has been thoroughly reviewed now. Committed, thanks! Thank you, Heikki. And also big thanks to Fabien for the review! .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
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
---
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hello Marko,
Here's a patch for making PL/PgSQL throw an error during compilation (instead
of runtime) if the number of parameters passed to RAISE don't match the
number of placeholders in error message. I'm sure people can see the pros of
doing it this way.
Patch scanned, applied tested without problem on head.
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 would suggest to update the documentation accordingly.
- The execution code now contains error detections which should never be
raised, but I suggest to keep it in place anyway. However I would suggest
to add comments about the fact that it should not be triggered.
See the attached patch which implements these suggestions on top of your
patch.
--
Fabien.diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 91fadb0..a3c763a 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3403,6 +3403,8 @@ RAISE ;
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 optional arguments must match the number of literal%/
+placeholders in the format string, otherwise a compilation error is reported.
/para
para
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 69d1965..a3cd6fc 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2939,6 +2939,10 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
continue;
}
+/*
+ * Too few arguments to raise:
+ * This should never happen as a compile-time check is performed.
+ */
if (current_param == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -2965,7 +2969,8 @@ exec_stmt_raise(PLpgSQL_execstate *estate, PLpgSQL_stmt_raise *stmt)
/*
* If more parameters were specified than were required to process the
- * format string, throw an error
+ * format string, throw an error.
+ * This should never happen as a compile-time check is performed.
*/
if (current_param != NULL)
ereport(ERROR,
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index ba928e3..03976e4 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -3785,15 +3785,12 @@ check_raise_parameters(PLpgSQL_stmt_raise *stmt)
for (cp = stmt-message; *cp; cp++)
{
- if (cp[0] != '%')
- continue;
- /* literal %, ignore */
- if (cp[1] == '%')
- {
- cp++;
- continue;
+ if (cp[0] == '%') {
+ if (cp[1] == '%') /* literal %, ignore */
+cp++;
+ else
+expected_nparams++;
}
- expected_nparams++;
}
if (expected_nparams list_length(stmt-params))
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hi Fabien, On 8/12/14 1:09 PM, Fabien COELHO wrote: Here's a patch for making PL/PgSQL throw an error during compilation (instead of runtime) if the number of parameters passed to RAISE don't match the number of placeholders in error message. I'm sure people can see the pros of doing it this way. Patch scanned, applied tested without problem on head. Thanks! 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 would suggest to update the documentation accordingly. I scanned the docs trying to find any mentions of the run-time error but couldn't find any. I don't object to adding this, though. - The execution code now contains error detections which should never be raised, but I suggest to keep it in place anyway. However I would suggest to add comments about the fact that it should not be triggered. Good point. I actually realized I hadn't sent the latest version of the patch, sorry about that. I did this: https://github.com/johto/postgres/commit/1acab6fc5387c893ca29fed9284e09258e0c5c56 to also turn the ereport()s into elog()s since the user should never see them. See the attached patch which implements these suggestions on top of your patch. Thanks for reviewing! I'll incorporate the doc changes, but I'm going to let others decide on the logic in the check_raise_parameters() loop before doing any changes. Will send an updated patch shortly. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hello,
- 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.
Hmmm. I had to read the code to check it, and I did it twice. The point is
that there is 3 exit points instead of 1 in the block, which is not in
itself very bad, but:
for(...) {
if (ccc)
xxx;
...
foo++;
}
It looks like foo++ is always executed, and you have to notice that xxx
a few lines above is a continue to realise that it is only when ccc is
false. This is also disconcerting if it happens to be the rare case,
i.e. here most of the time the char is not '%', so most of the time foo is
not incremented, although it is a the highest level. Also the code with
continue does not really put forward that what is counted is '%' followed
by a non '%'. Note that the corresponding execution code
(pgplsql/src/pl_exec.c) uses one continue to get rid of the second '%',
which is quite defendable in that case as it is a kind of exception, but
the main condition remains a simple if block. Final argument, the
structured version is shorter than the unstructured version, with just the
two continues removed, and one negation also removed.
to also turn the ereport()s into elog()s since the user should never see
them.
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.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
2014-08-12 15:09 GMT+02:00 Fabien COELHO coe...@cri.ensmp.fr:
Hello,
- 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.
Hmmm. I had to read the code to check it, and I did it twice. The point is
that there is 3 exit points instead of 1 in the block, which is not in
itself very bad, but:
for(...) {
if (ccc)
xxx;
...
foo++;
}
It looks like foo++ is always executed, and you have to notice that xxx
a few lines above is a continue to realise that it is only when ccc is
false. This is also disconcerting if it happens to be the rare case, i.e.
here most of the time the char is not '%', so most of the time foo is not
incremented, although it is a the highest level. Also the code with
continue does not really put forward that what is counted is '%' followed
by a non '%'. Note that the corresponding execution code
(pgplsql/src/pl_exec.c) uses one continue to get rid of the second '%',
which is quite defendable in that case as it is a kind of exception, but
the main condition remains a simple if block. Final argument, the
structured version is shorter than the unstructured version, with just the
two continues removed, and one negation also removed.
to also turn the ereport()s into elog()s since the user should never see
them.
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.
one note: this patch can enforce a compatibility issues - a partially
broken functions, where some badly written RAISE statements was executed
newer.
I am not against this patch, but it should be in extra check probably ?? Or
we have to documented it as potential compatibility issue
Regards
Pavel
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
one note: this patch can enforce a compatibility issues - a partially broken functions, where some badly written RAISE statements was executed newer. I am not against this patch, but it should be in extra check probably ?? I'm not sure about what you mean by it should be in extra check. Or we have to documented it as potential compatibility issue. Indeed, as a potential execution error is turned into a certain compilation error. If this compatibility point is a blocker, the compilation error can be turned into a warning, but I would prefer to keep it an error: I'm quite sure I fell into that pit at least once or twice. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
2014-08-12 19:14 GMT+02:00 Fabien COELHO coe...@cri.ensmp.fr: one note: this patch can enforce a compatibility issues - a partially broken functions, where some badly written RAISE statements was executed newer. I am not against this patch, but it should be in extra check probably ?? I'm not sure about what you mean by it should be in extra check. Or we have to documented it as potential compatibility issue. Indeed, as a potential execution error is turned into a certain compilation error. If this compatibility point is a blocker, the compilation error can be turned into a warning, but I would prefer to keep it an error: I'm quite sure I fell into that pit at least once or twice. I prefer error, although there is possibility to control a force of exception - error or warning via extra plpgsql checks - this kind of error is strange - and stored procedures usually can be fixed later because plpgsql source is available. There was a precedent with more precious query syntax check more times. Just it should be well documented. Regards Pavel -- Fabien.
[HACKERS] PL/PgSQL: RAISE and the number of parameters
Me again,
Here's a patch for making PL/PgSQL throw an error during compilation
(instead of runtime) if the number of parameters passed to RAISE don't
match the number of placeholders in error message. I'm sure people can
see the pros of doing it this way.
.marko
*** 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
staticPLpgSQL_expr*read_cursor_args(PLpgSQL_var *cursor,
int until, const char *expected);
staticList*read_raise_options(void);
+ staticvoid
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,3812
}
/*
+ * 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;
+ /* literal %, ignore */
+ if (cp[1] == '%')
+ {
+ cp++;
+ continue;
+ }
+ 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
Re: [HACKERS] PL/PgSQL: RAISE and the number of parameters
Hi 2014-07-26 20:39 GMT+02:00 Marko Tiikkaja ma...@joh.to: Me again, Here's a patch for making PL/PgSQL throw an error during compilation (instead of runtime) if the number of parameters passed to RAISE don't match the number of placeholders in error message. I'm sure people can see the pros of doing it this way. +1 Regards Pavel .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
