Thread: Improving some plpgsql error messages
In commit 2f48ede08 I copied some error-message wording that had existed in exec_run_select() since 2003. I'm now dissatified with that, after realizing that it can produce output like this: ERROR: query "WITH a AS ( SELECT regexp_split_to_table(info_out, '\n') AS info FROM public.results WHERE public.results.jid = id ) SELECT * INTO tabular_info FROM a RETURN" is not a SELECT CONTEXT: PL/pgSQL function get_info(text) line 3 at RETURN QUERY There are a couple of things wrong with this: 1. The complaint is factually inaccurate, since the query obviously *is* a SELECT. It's the INTO part that's the problem, but good luck guessing that from the error text. (See [1] for motivation.) 2. It's fairly unreadable when the query is a long multi-line one, as it does a great job of burying the lede. This also violates our message style guideline that says primary error messages should be one-liners. The way to fix #1 is to provide a special case for SELECT INTO. In the attached I propose to fix #2 by moving the query text into an errcontext line, thus producing something like ERROR: query is SELECT INTO, but it should be plain SELECT CONTEXT: query: WITH a AS ( SELECT regexp_split_to_table(info_out, '\n') AS info FROM public.results WHERE public.results.jid = id ) SELECT * INTO tabular_info FROM a RETURN PL/pgSQL function get_info(text) line 3 at RETURN QUERY A case could also be made for just dropping the query text entirely, but I'm inclined to think that's not such a great idea. Certainly we ought to show the text in case of RETURN QUERY EXECUTE, where it won't be embedded in the function text. Looking around, I noted that exec_eval_expr() also has the bad habit of quoting the expression text right in the primary message, so the attached fixes that too. That case is slightly less bad since expressions are more likely to be short, but they surely aren't all short. Thoughts? Should I back-patch this into v14 where 2f48ede08 came in, or just do it in HEAD? regards, tom lane [1] https://www.postgresql.org/message-id/y65a6lco0z4.fsf%40mun.ca diff --git a/src/pl/plpgsql/src/expected/plpgsql_array.out b/src/pl/plpgsql/src/expected/plpgsql_array.out index 5f28b4f685..9e22e56f00 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_array.out +++ b/src/pl/plpgsql/src/expected/plpgsql_array.out @@ -73,8 +73,9 @@ PL/pgSQL function inline_code_block line 2 at assignment insert into onecol values(array[11]); do $$ declare a int[]; begin a := f1 from onecol; raise notice 'a = %', a; end$$; -ERROR: query "a := f1 from onecol" returned more than one row -CONTEXT: PL/pgSQL function inline_code_block line 2 at assignment +ERROR: query returned more than one row +CONTEXT: query: a := f1 from onecol +PL/pgSQL function inline_code_block line 2 at assignment do $$ declare a int[]; begin a := f1 from onecol limit 1; raise notice 'a = %', a; end$$; NOTICE: a = {1,2} diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 14bbe12da5..7c5bc63778 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -3557,9 +3557,22 @@ exec_stmt_return_query(PLpgSQL_execstate *estate, rc = SPI_execute_plan_extended(expr->plan, &options); if (rc != SPI_OK_SELECT) - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("query \"%s\" is not a SELECT", expr->query))); + { + /* + * SELECT INTO deserves a special error message, because "query is + * not a SELECT" is not very helpful in that case. + */ + if (rc == SPI_OK_SELINTO) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("query is SELECT INTO, but it should be plain SELECT"), + errcontext("query: %s", expr->query))); + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("query is not a SELECT"), + errcontext("query: %s", expr->query))); + } } else { @@ -5644,7 +5657,8 @@ exec_eval_expr(PLpgSQL_execstate *estate, if (rc != SPI_OK_SELECT) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("query \"%s\" did not return data", expr->query))); + errmsg("query did not return data"), + errcontext("query: %s", expr->query))); /* * Check that the expression returns exactly one column... @@ -5652,11 +5666,11 @@ exec_eval_expr(PLpgSQL_execstate *estate, if (estate->eval_tuptable->tupdesc->natts != 1) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg_plural("query \"%s\" returned %d column", - "query \"%s\" returned %d columns", + errmsg_plural("query returned %d column", + "query returned %d columns", estate->eval_tuptable->tupdesc->natts, - expr->query, - estate->eval_tuptable->tupdesc->natts))); + estate->eval_tuptable->tupdesc->natts), + errcontext("query: %s", expr->query))); /* * ... and get the column's datatype. @@ -5680,8 +5694,8 @@ exec_eval_expr(PLpgSQL_execstate *estate, if (estate->eval_processed != 1) ereport(ERROR, (errcode(ERRCODE_CARDINALITY_VIOLATION), - errmsg("query \"%s\" returned more than one row", - expr->query))); + errmsg("query returned more than one row"), + errcontext("query: %s", expr->query))); /* * Return the single result Datum. @@ -5748,9 +5762,22 @@ exec_run_select(PLpgSQL_execstate *estate, rc = SPI_execute_plan_with_paramlist(expr->plan, paramLI, estate->readonly_func, maxtuples); if (rc != SPI_OK_SELECT) - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("query \"%s\" is not a SELECT", expr->query))); + { + /* + * SELECT INTO deserves a special error message, because "query is not + * a SELECT" is not very helpful in that case. + */ + if (rc == SPI_OK_SELINTO) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("query is SELECT INTO, but it should be plain SELECT"), + errcontext("query: %s", expr->query))); + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("query is not a SELECT"), + errcontext("query: %s", expr->query))); + } /* Save query results for eventual cleanup */ Assert(estate->eval_tuptable == NULL);
On Fri, Aug 20, 2021 at 8:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thoughts? Should I back-patch this into v14 where 2f48ede08
came in, or just do it in HEAD?
I'd say back-patch in the interest of preventing probably quite a few emails from novices at plpgsql coding dealing with all the interplay between queries and variables and returning syntaxes.
David J.
pá 20. 8. 2021 v 17:50 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
In commit 2f48ede08 I copied some error-message wording that had
existed in exec_run_select() since 2003. I'm now dissatified
with that, after realizing that it can produce output like this:
ERROR: query "WITH a AS (
SELECT
regexp_split_to_table(info_out, '\n') AS info
FROM
public.results
WHERE
public.results.jid = id
)
SELECT
* INTO tabular_info
FROM
a RETURN" is not a SELECT
CONTEXT: PL/pgSQL function get_info(text) line 3 at RETURN QUERY
There are a couple of things wrong with this:
1. The complaint is factually inaccurate, since the query obviously
*is* a SELECT. It's the INTO part that's the problem, but good luck
guessing that from the error text. (See [1] for motivation.)
2. It's fairly unreadable when the query is a long multi-line one,
as it does a great job of burying the lede. This also violates
our message style guideline that says primary error messages should
be one-liners.
The way to fix #1 is to provide a special case for SELECT INTO.
In the attached I propose to fix #2 by moving the query text into
an errcontext line, thus producing something like
ERROR: query is SELECT INTO, but it should be plain SELECT
CONTEXT: query: WITH a AS (
SELECT
regexp_split_to_table(info_out, '\n') AS info
FROM
public.results
WHERE
public.results.jid = id
)
SELECT
* INTO tabular_info
FROM
a RETURN
PL/pgSQL function get_info(text) line 3 at RETURN QUERY
A case could also be made for just dropping the query text entirely,
but I'm inclined to think that's not such a great idea. Certainly
we ought to show the text in case of RETURN QUERY EXECUTE, where it
won't be embedded in the function text.
Looking around, I noted that exec_eval_expr() also has the bad
habit of quoting the expression text right in the primary message,
so the attached fixes that too. That case is slightly less bad
since expressions are more likely to be short, but they surely
aren't all short.
Thoughts? Should I back-patch this into v14 where 2f48ede08
came in, or just do it in HEAD?
It can be fixed in 14. This is a low risk patch.
Regards
Pavel
regards, tom lane
[1] https://www.postgresql.org/message-id/y65a6lco0z4.fsf%40mun.ca
On Fri, Aug 20, 2021 at 12:41 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > It can be fixed in 14. This is a low risk patch. +1. -- Robert Haas EDB: http://www.enterprisedb.com