Обсуждение: [proposal] Add an option for returning SQLSTATE in psql error message

Hi,

Currently on error psql is printing Postgres' PQerrorMessage text, but
there's no guarantee these messages are constant between Postgres
versions and it's a pain when using psql for writing regression tests,

Prior
  Disallow setting client_min_messages higher than ERROR.

a bad workaround was to discarded error, now you have to do something like
CREATE OR REPLACE FUNCTION catch_error(
    query text
)
    RETURNS void AS $$
DECLARE
BEGIN
        EXECUTE query;
        EXCEPTION WHEN OTHERS THEN
            RAISE 'Query failed: %', SQLSTATE;
END;
$$LANGUAGE plpgsql;

SELECT catch_error('foo');

What about a new \whatever for setting psql error to either PQerrorMessage or
PQresultErrorField(res, PG_DIAG_SQLSTATE) if available?

didier <did447@gmail.com> writes:
> Currently on error psql is printing Postgres' PQerrorMessage text, but
> there's no guarantee these messages are constant between Postgres
> versions and it's a pain when using psql for writing regression tests,

I don't buy that argument.  We use psql's normal display in all the
regular regression tests, and it's not a big maintenance problem.
If the error message changes, that's usually interesting in itself.

Also, if it does change, it's often because you're hitting a different
error-detection test, which more than likely is throwing a different
SQLSTATE anyway.

Lastly, because the SQL spec has been rather miserly in assigning
SQLSTATEs in some areas, there are lots of cases where the same
SQLSTATE is used for several distinct errors; for instance
ERRCODE_UNDEFINED_OBJECT covers a lot of ground.  Do you really
want your test cases to be unable to distinguish those?

There might be a use-case for showing only SQLSTATE, but writing
regression tests doesn't seem like a good example.

            regards, tom lane

Currently with 10 head
ERROR:  could not determine polymorphic type because input has type unknown
With 9 head
ERROR:  could not determine polymorphic type because input has type "unknown"

Another option could be: set display error to none and let user's
script do some regular expression on pdsl variable.
On Sun, Dec 2, 2018 at 5:05 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> ne 2. 12. 2018 v 16:56 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>>
>> didier <did447@gmail.com> writes:
>> > Currently on error psql is printing Postgres' PQerrorMessage text, but
>> > there's no guarantee these messages are constant between Postgres
>> > versions and it's a pain when using psql for writing regression tests,
>>
>> I don't buy that argument.  We use psql's normal display in all the
>> regular regression tests, and it's not a big maintenance problem.
>> If the error message changes, that's usually interesting in itself.
>
>
> our tests are not against different PostgreSQL releases. When you have a code, that should to support more PostgreSQL
releases,then it is sometimes difficult. 
>
>>
>> Also, if it does change, it's often because you're hitting a different
>> error-detection test, which more than likely is throwing a different
>> SQLSTATE anyway.
>>
>> Lastly, because the SQL spec has been rather miserly in assigning
>> SQLSTATEs in some areas, there are lots of cases where the same
>> SQLSTATE is used for several distinct errors; for instance
>> ERRCODE_UNDEFINED_OBJECT covers a lot of ground.  Do you really
>> want your test cases to be unable to distinguish those?
>>
>> There might be a use-case for showing only SQLSTATE, but writing
>> regression tests doesn't seem like a good example.
>>
>>                         regards, tom lane
>>

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> I don't buy that argument. We use psql's normal display in all the
 Tom> regular regression tests, and it's not a big maintenance problem.

The regular regression tests have the advantage that they don't need to
work across pg versions.

It is more of a problem for extensions; I just ran into this myself in
fact, with a test failing because "invalid input syntax for integer" got
changed to "invalid input syntax for type integer".

-- 
Andrew (irc:RhodiumToad)

Attached a POC adding a new variable ECHO_ERROR
\set ECHO_ERROR text|none|psqlstate

On Mon, Dec 3, 2018 at 2:47 AM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>  Tom> I don't buy that argument. We use psql's normal display in all the
>  Tom> regular regression tests, and it's not a big maintenance problem.
>
> The regular regression tests have the advantage that they don't need to
> work across pg versions.
>
> It is more of a problem for extensions; I just ran into this myself in
> fact, with a test failing because "invalid input syntax for integer" got
> changed to "invalid input syntax for type integer".
>
> --
> Andrew (irc:RhodiumToad)

>>>>> "didier" == didier  <did447@gmail.com> writes:

 didier> Attached a POC adding a new variable ECHO_ERROR
 didier> \set ECHO_ERROR text|none|psqlstate

I wouldn't have called it that. Possibly another option to the existing
VERBOSITY variable? \set VERBOSITY sqlstate_only or something of that
ilk (it's already not unusual to use \set VERBOSITY terse in regression
tests)

-- 
Andrew (irc:RhodiumToad)

Yep, name is bad, but I'm not sure about VERBOSITY, isn't it
controlling output from the server not the client?
You may want to set both VERBOSITY to 'verbose' and ECHO_ERROR to
'none' then in script do
SELECT .... -- no error output
\if :ERROR
   -- do something with LAST_ERROR_MESSAGE



On Mon, Dec 3, 2018 at 4:49 PM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>
> >>>>> "didier" == didier  <did447@gmail.com> writes:
>
>  didier> Attached a POC adding a new variable ECHO_ERROR
>  didier> \set ECHO_ERROR text|none|psqlstate
>
> I wouldn't have called it that. Possibly another option to the existing
> VERBOSITY variable? \set VERBOSITY sqlstate_only or something of that
> ilk (it's already not unusual to use \set VERBOSITY terse in regression
> tests)
>
> --
> Andrew (irc:RhodiumToad)

didier <did447@gmail.com> writes:
> Yep, name is bad, but I'm not sure about VERBOSITY, isn't it
> controlling output from the server not the client?

No, it's in libpq, so you'd have to touch that not the server.

I agree with Andrew's thought, and would further say that just
"\set VERBOSITY sqlstate" would be a reasonable API.  Making this
separate from VERBOSITY is weird.

            regards, tom lane

On Mon, Dec 3, 2018 at 5:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> No, it's in libpq, so you'd have to touch that not the server.
libpq, not as bad as the server but nonetheless maybe a bit too much for this.
Is adding value in library contract?

Anyway. attached a POC adding a new value to VERBOSITY (hopefully
nobody is using it).

On Mon, Dec 3, 2018 at 7:02 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> It can works :). Please, assign it to next commitfest.

Ok

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

This is a handy feature, and the implementation looks good to me.

There might be some nit-picking about the vertical whitespace around
the code in added to fe-protocol3.c, but I'll leave that to the committer.

The new status of this patch is: Ready for Committer

On 04/12/2018 13:18, didier wrote:
> diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
> index 1bb2a6e16d..64628f29a3 100644
> --- a/src/test/regress/sql/psql.sql
> +++ b/src/test/regress/sql/psql.sql
> @@ -1016,3 +1016,22 @@ select 1/(15-unique2) from tenk1 order by unique2 limit 19;
>  \echo 'last error code:' :LAST_ERROR_SQLSTATE
>  
>  \unset FETCH_COUNT
> +
> +-- verbosity error setting
> +\set VERBOSITY terse
> +SELECT 1 UNION;
> +\echo 'error:' :ERROR
> +\echo 'error code:' :SQLSTATE
> +\echo 'last error message:' :LAST_ERROR_MESSAGE
> +
> +\set VERBOSITY sqlstate
> +SELECT 1 UNION;
> +\echo 'error:' :ERROR
> +\echo 'error code:' :SQLSTATE
> +\echo 'last error message:' :LAST_ERROR_MESSAGE
> +
> +\set VERBOSITY default
> +SELECT 1 UNION;
> +\echo 'error:' :ERROR
> +\echo 'error code:' :SQLSTATE
> +\echo 'last error message:' :LAST_ERROR_MESSAGE
> -- 

Why are you not including a test for \set VERBOSITY verbose?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Why are you not including a test for \set VERBOSITY verbose?

Stability of the output would be a problem ...

            regards, tom lane

On Mon, Jan 07, 2019 at 10:44:24PM +0100, didier wrote:
> On Sat, Jan 5, 2019 at 6:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> Why are you not including a test for \set VERBOSITY verbose?
>>
>> Stability of the output would be a problem ...
>>
>> Yes it could moreover the functionality wasn't tested before.
> Should I add one ?

Unpredictible outputs mean more maintenance and more alternate
outputs.  I have moved the patch to next CF, still ready for
committer.
--
Michael

On 2/4/19 5:54 AM, Michael Paquier wrote:
> On Mon, Jan 07, 2019 at 10:44:24PM +0100, didier wrote:
>> On Sat, Jan 5, 2019 at 6:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>>> Why are you not including a test for \set VERBOSITY verbose?
>>>
>>> Stability of the output would be a problem ...
>>>
>>> Yes it could moreover the functionality wasn't tested before.
>> Should I add one ?
> 
> Unpredictible outputs mean more maintenance and more alternate
> outputs.  I have moved the patch to next CF, still ready for
> committer.

What do you think, Peter?  Is the extra test valuable or will it cause 
unpredictable outputs as Tom and Michael predict?

Regards,
-- 
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
>>>>> Why are you not including a test for \set VERBOSITY verbose?

> What do you think, Peter?  Is the extra test valuable or will it cause 
> unpredictable outputs as Tom and Michael predict?

I'm not really sure why this is open for discussion.

regression=# \set VERBOSITY verbose
regression=# select 1/0;
ERROR:  22012: division by zero
LOCATION:  int4div, int.c:824

It's not going to be tolerable to have to adjust such a test anytime
somebody adds or removes lines in whichever backend file throws the
tested-for error (never mind more-substantial refactoring such as
moving the ereport call to a different function or file).  I also
believe that the reported line number will vary across compilers
even without that: IME you might get either the starting or ending
line number of the ereport() construct.

There's also the question of whether the function name is reliably
available.  Maybe it is now that we require C99 compatibility,
but the code hasn't been taught to assume that.

            regards, tom lane

On Thursday, March 21, 2019 7:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> David Steele david@pgmasters.net writes:
>
> > > > > > Why are you not including a test for \set VERBOSITY verbose?
>
> > What do you think, Peter? Is the extra test valuable or will it cause
> > unpredictable outputs as Tom and Michael predict?
>
> I'm not really sure why this is open for discussion.
>
> regression=# \set VERBOSITY verbose
> regression=# select 1/0;
> ERROR: 22012: division by zero
> LOCATION: int4div, int.c:824
>
> It's not going to be tolerable to have to adjust such a test anytime
> somebody adds or removes lines in whichever backend file throws the
> tested-for error (never mind more-substantial refactoring such as
> moving the ereport call to a different function or file). I also
> believe that the reported line number will vary across compilers
> even without that: IME you might get either the starting or ending
> line number of the ereport() construct.

In GPDB we have to handle variable output in tests similar to this (but
for very different reasons), and unless there is a big gain elsewhere
from having variable test output I would advise against it. It adds a
fair bit of complexity and another moving part which can mask subtle
test failures.

cheers ./daniel

On 2019-03-21 19:01, David Steele wrote:
> What do you think, Peter?  Is the extra test valuable or will it cause 
> unpredictable outputs as Tom and Michael predict?

Yes, I'm OK with that.

But now that I read the patch again, I'm not sure why this needs to
touch libpq.  The formatting of error messages in psql should be handled
in psql.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> But now that I read the patch again, I'm not sure why this needs to
> touch libpq.  The formatting of error messages in psql should be handled
> in psql.

Maybe in an ideal world that'd be the case, but psql has always just
depended on PQerrorMessage().  I don't think this patch should be
tasked with changing that.  I'm not even sure that doing so would be
a good idea: I think the idea there is that whatever we believe is a
nice error-reporting option for psql ought to be easily available to
other libpq clients too.

(Note: I haven't read the patch and don't mean to be saying that it's
necessarily OK.  But I'm fine with the idea that something like this
involves touching libpq more than psql.)

            regards, tom lane

didier <did447@gmail.com> writes:
> [ 0001-Add-sqlstate-output-mode-to-VERBOSITY_v1.patch ]

Pushed with some mostly-cosmetic adjustments.  The main non-cosmetic
thing I did was to adjust the logic so that if no SQLSTATE is available,
it acts like TERSE mode.  Otherwise, we'd print nothing at all except
"ERROR:", which seems both quite useless and contrary to our message
style guidelines.  Moreover, documenting it this way makes the behavior
not inconsistent with what happens for libpq-generated errors and
errors from protocol-version-2 servers.

            regards, tom lane