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

ne 2. 12. 2018 v 15:34 odesílatel didier <did447@gmail.com> 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

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

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.

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.

"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:

"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)

po 3. 12. 2018 v 16:49 odesílatel Andrew Gierth <andrew@tao11.riddles.org.uk>
napsal:

"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)

It is good idea to look to this option.

Pavel

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 <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).

po 3. 12. 2018 v 18:57 odesílatel didier <did447@gmail.com> napsal:

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).

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

Regards

Pavel

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

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