On Mon, Feb 8, 2016 at 5:24 PM, Daniel Verite <dan...@manitou-mail.org> wrote:
> 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. > I've edited the CF entry title to read: Add \errverbose to psql (and support in libpq) 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? > Not exactly, the verbosity setting might or might not be in effect when this function is called. Another external part of the state that might affect the result is conn->show_context field. 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. > This is because PQresultErrorMessage() returns a stored error message: res->errMsg (or ""). - 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. > For the same reason: this function actually produces a new error message, as opposed to returning a stored one. - 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? > Hm, probably we could just copy the message from conn->errorMessage, in case of protocol v2, but I don't see a point in passing PGresult to the func or naming it PQresult<Something> in the case of v2. - if the fixed error message is kept, it should pass through > libpq_gettext() for translation. > Good point. - 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-created ones." > > As it's precisely the point of that new function, that bit could > be altered to refer to it. > I didn't touch the documentation specifically, because this is just a proof-of-concept, but it's good that you notice this detail. Most importantly, I'd like to learn of better options than storing the whole last_result in psql's pset structure. Thanks for the review! -- Alex
