Re: Improving some plpgsql error messages - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: Improving some plpgsql error messages
Date
Msg-id CAFj8pRDmj9akj9xaAcYhiAErdYanGpc=7fmJCYeKgMKb0Asa3g@mail.gmail.com
Whole thread Raw
In response to Improving some plpgsql error messages  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Improving some plpgsql error messages  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers


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

pgsql-hackers by date:

Previous
From: "Bossart, Nathan"
Date:
Subject: Re: archive status ".ready" files may be created too early
Next
From: Robert Haas
Date:
Subject: Re: archive status ".ready" files may be created too early