[proposal] Add an option for returning SQLSTATE in psql error message

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[proposal] Add an option for returning SQLSTATE in psql error message

didier
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?

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Pavel Stehule


ne 2. 12. 2018 v 15:34 odesílatel didier <[hidden email]> napsal:
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?


It looks weird. Maybe we can define a option where only SQL state will be displayed.

Regards

Pavel
Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Tom Lane-2
In reply to this post by didier
didier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Pavel Stehule


ne 2. 12. 2018 v 16:56 odesílatel Tom Lane <[hidden email]> napsal:
didier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

didier
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 <[hidden email]> wrote:

>
>
>
> ne 2. 12. 2018 v 16:56 odesílatel Tom Lane <[hidden email]> napsal:
>>
>> didier <[hidden email]> 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
>>

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Andrew Gierth
In reply to this post by Tom Lane-2
>>>>> "Tom" == Tom Lane <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

didier
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
<[hidden email]> wrote:

>
> >>>>> "Tom" == Tom Lane <[hidden email]> 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)

master_psql_error_output.diff (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Andrew Gierth
>>>>> "didier" == didier  <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Pavel Stehule


po 3. 12. 2018 v 16:49 odesílatel Andrew Gierth <[hidden email]> napsal:
>>>>> "didier" == didier  <[hidden email]> 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)

It is good idea to look to this option.

Pavel


--
Andrew (irc:RhodiumToad)

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

didier
In reply to this post by Andrew Gierth
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
<[hidden email]> wrote:

>
> >>>>> "didier" == didier  <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Tom Lane-2
didier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

didier
On Mon, Dec 3, 2018 at 5:51 PM Tom Lane <[hidden email]> 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).

master_verbosity_sqlstate.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Pavel Stehule


po 3. 12. 2018 v 18:57 odesílatel didier <[hidden email]> napsal:
On Mon, Dec 3, 2018 at 5:51 PM Tom Lane <[hidden email]> 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).

It can works :). Please, assign it to next commitfest.

Regards

Pavel

Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

didier
On Mon, Dec 3, 2018 at 7:02 PM Pavel Stehule <[hidden email]> wrote:
> It can works :). Please, assign it to next commitfest.

Ok

0001-Add-sqlstate-output-mode-to-VERBOSITY_v1.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [proposal] Add an option for returning SQLSTATE in psql error message

Dagfinn Ilmari Mannsåker
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