Re: Add schema-qualified relnames in constraint error messages. - Mailing list pgsql-hackers

Shulgin, Oleksandr wrote:

> Added to the Open commitfest: https://commitfest.postgresql.org/9/475/

Here's a review. Note that the patch tested and submitted
is not the initial one in the thread, so it doesn't exactly
match  $subject now.
What's tested here is a client-side approach, suggested by Tom
upthread as an alternative, and implemented by Oleksandr in
0001-POC-errverbose-in-psql.patch

The patch has two parts: one part in libpq exposing a new function
named PQresultBuildErrorMessage, and a second part implementing an
\errverbose command in psql, essentially displaying the result of the
function.
The separation makes sense if we consider that other clients might benefit
from the function, or that libpq is a better place than psql to maintain
this in the future, as the list of error fields available in a PGresult
might grow.
OTOH maybe adding a new libpq function just for that is overkill, but this
is subjective.

psql part
=========
Compiles and works as intended.
For instance with \set VERBOSITY default, an FK violation produces:
 # insert into table2 values(10); ERROR:  insert or update on table "table2" violates foreign key constraint
"table2_id_tbl1_fkey"DETAIL:  Key (id_tbl1)=(10) is not present in table "table1". 

Then \errverbose just displays the verbose form of the error: # \errverbose   ERROR:  23503: insert or update on table
"table2"violates foreign     key constraint "table2_id_tbl1_fkey"   DETAIL:  Key (id_tbl1)=(10) is not present in table
"table1".  SCHEMA NAME:  public   TABLE NAME:  table2   CONSTRAINT NAME:  table2_id_tbl1_fkey   LOCATION:
ri_ReportViolation,ri_triggers.c:3326 

- make check passes
- valgrind test is OK (no obvious leak after using the command).

Missing bits:
- the command is not mentioned in the help (\? command, see help.c)
- it should be added to tab completions (see tab-complete.c)
- it needs to be described in the SGML documentation

libpq part
==========
The patch implements and exports a new PQresultBuildErrorMessage()
function.

AFAICS its purpose is to produce a result similar to what
PQresultErrorMessage() would have produced, if PQERRORS_VERBOSE
was the verbosity in effect at execution time.

My comments:

- the name of the function does not really hint at what it does.
Maybe something like PQresultVerboseErrorMessage() would be more
explicit?

I would expect the new function to have the same interface than
PQresultErrorMessage(), but it's not the case.

- it takes a PGconn* and PGresult* as input parameters, but
PQresultErrorMessage takes only a <const PGresult*> as input.
It's not clear why PGconn* is necessary.

- it returns a pointer to a strdup'ed() buffer, which
has to be freed separately by the caller. That differs from
PQresultErrorMessage() which keeps this managed inside the
PGresult struct.

- if protocol version < 3, an error message is returned. It's not
clear to the caller that this error is emitted by the function itself
rather than the query. Besides, are we sure it's necessary?
Maybe the function could just produce an output with whatever
error fields it has, even minimally with older protocol versions,
rather than failing?

- if the fixed error message is kept, it should pass through
libpq_gettext() for translation.

- a description of the function should be added to the SGML doc.
There's a sentence in PQsetErrorVerbosity() that says, currently:
 "Changing the verbosity does not affect the messages available from  already-existing PGresult objects, only
subsequently-createdones." 

As it's precisely the point of that new function, that bit could
be altered to refer to it.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite