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
Attachments:
raise_check_number_of_arguments.v1.patchtext/plain; charset=UTF-8; name=raise_check_number_of_arguments.v1.patch; x-mac-creator=0; x-mac-type=0Download
*** 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,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 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.
Hi
2014-07-26 20:39 GMT+02:00 Marko Tiikkaja <marko@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
Show quoted text
.marko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Attachments:
raise_check_v1-suggestion.patchtext/x-diff; charset=us-ascii; name=raise_check_v1-suggestion.patchDownload
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))
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
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
2014-08-12 15:09 GMT+02:00 Fabien COELHO <coelho@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
Show quoted text
--
Fabien.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
2014-08-12 19:14 GMT+02:00 Fabien COELHO <coelho@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
Show quoted text
--
Fabien.
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
Attachments:
raise_check_number_of_arguments.v2.patchtext/x-patch; name=raise_check_number_of_arguments.v2.patchDownload
*** 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.
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
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
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