Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Sun, Apr 3, 2016, 18:32 Tom Lane wrote: > Peter Geoghegan writes: > > On Thu, Feb 11, 2016 at 9:50 AM, Robert Haas > wrote: > >> Wow, that's a fabulous idea. I see Oleksandr has tried to implement > >> it, although I haven't looked at the patch. But I think this would be > >> REALLY helpful. > > > +1 > > Pushed. > Yay!
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Peter Geoghegan writes: > On Thu, Feb 11, 2016 at 9:50 AM, Robert Haas wrote: >> Wow, that's a fabulous idea. I see Oleksandr has tried to implement >> it, although I haven't looked at the patch. But I think this would be >> REALLY helpful. > +1 Pushed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Alex Shulgin writes: > What about regression tests? My assumption was that we won't be able to > add them with the usual expected file approach, but that we also don't need > it that hard. Everyone's in favor? It'd be nice to have a regression test, but I concur that the LOCATION information is too unstable for that to be practical. We could imagine testing some variant behavior that omits printing LOCATION, but that doesn't really seem worth the trouble; I think we'd be inventing the variant behavior only for testing purposes. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Sat, Apr 2, 2016 at 11:41 PM, Tom Lane wrote: > "Shulgin, Oleksandr" writes: > > On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane wrote: > >> Yeah, I don't much like that either. But I don't think we can avoid > >> some refactoring there; as designed, conversion of an error message into > >> user-visible form is too tightly tied to receipt of the message. > > > True. Attached is a v2 which addresses all of the points raised earlier > I > > believe. > > I took a closer look at what's going on here and realized that actually > it's not that hard to decouple the message-building routine from the > PGconn state, because mostly it works with fields it's extracting out > of the PGresult anyway. The only piece of information that's lacking > is conn->last_query. I propose therefore that instead of doing it like > this, we copy last_query into error PGresults. This is strictly less > added storage requirement than storing the whole verbose message would be, > and it saves time compared to the v2 patch in the typical case where > the application never does ask for an alternately-formatted error message. > Plus we can actually support requests for any variant format, not only > VERBOSE. > > Attached is a libpq-portion-only version of a patch doing it this way. > I've not yet looked at the psql part of your patch. > > Comments? > Ah, neat, that's even better. :-) What about regression tests? My assumption was that we won't be able to add them with the usual expected file approach, but that we also don't need it that hard. Everyone's in favor? -- Alex
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Thu, Feb 11, 2016 at 9:50 AM, Robert Haas wrote: >> Actually, what'd be really handy IMO is something to regurgitate the >> most recent error in verbose mode, without making a permanent session >> state change. Something like >> >> regression=# insert into bar values(1); >> ERROR: insert or update on table "bar" violates foreign key constraint >> "bar_f1_fkey" >> DETAIL: Key (f1)=(1) is not present in table "foo". >> regression=# \saywhat >> ERROR: 23503: insert or update on table "bar" violates foreign key >> constraint "bar_f1_fkey" >> DETAIL: Key (f1)=(1) is not present in table "foo". >> SCHEMA NAME: public >> TABLE NAME: bar >> CONSTRAINT NAME: bar_f1_fkey >> LOCATION: ri_ReportViolation, ri_triggers.c:3326 >> regression=# > > Wow, that's a fabulous idea. I see Oleksandr has tried to implement > it, although I haven't looked at the patch. But I think this would be > REALLY helpful. +1 -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
I wrote: > Attached is a libpq-portion-only version of a patch doing it this way. > I've not yet looked at the psql part of your patch. Here's an update for the psql side. regards, tom lane diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 385cb59..330127a 100644 *** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** Tue Oct 26 21:40:57 CEST 1999 *** 1718,1723 --- 1718,1737 + \errverbose + + + + Repeats the most recent server error or notice message at maximum + verbosity, as though VERBOSITY were set + to verbose and SHOW_CONTEXT were + set to always. + + + + + + \f [ string ] diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 50dc43b..3401b51 100644 *** a/src/bin/psql/command.c --- b/src/bin/psql/command.c *** exec_command(const char *cmd, *** 822,827 --- 822,849 } } + /* \errverbose -- display verbose message from last failed query */ + else if (strcmp(cmd, "errverbose") == 0) + { + if (pset.last_error_result) + { + char *msg; + + msg = PQresultVerboseErrorMessage(pset.last_error_result, + PQERRORS_VERBOSE, + PQSHOW_CONTEXT_ALWAYS); + if (msg) + { + psql_error("%s", msg); + PQfreemem(msg); + } + else + puts(_("out of memory")); + } + else + puts(_("There was no previous error.")); + } + /* \f -- change field separator */ else if (strcmp(cmd, "f") == 0) { diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 892058e..a2a07fb 100644 *** a/src/bin/psql/common.c --- b/src/bin/psql/common.c *** AcceptResult(const PGresult *result) *** 497,502 --- 497,529 } + /* + * ClearOrSaveResult + * + * If the result represents an error, remember it for possible display by + * \errverbose. Otherwise, just PQclear() it. + */ + static void + ClearOrSaveResult(PGresult *result) + { + if (result) + { + switch (PQresultStatus(result)) + { + case PGRES_NONFATAL_ERROR: + case PGRES_FATAL_ERROR: + if (pset.last_error_result) + PQclear(pset.last_error_result); + pset.last_error_result = result; + break; + + default: + PQclear(result); + break; + } + } + } + /* * PSQLexec *** PSQLexec(const char *query) *** 548,554 if (!AcceptResult(res)) { ! PQclear(res); res = NULL; } --- 575,581 if (!AcceptResult(res)) { ! ClearOrSaveResult(res); res = NULL; } *** PSQLexecWatch(const char *query, const p *** 590,596 if (!AcceptResult(res)) { ! PQclear(res); return 0; } --- 617,623 if (!AcceptResult(res)) { ! ClearOrSaveResult(res); return 0; } *** SendQuery(const char *query) *** 1077,1087 if (PQresultStatus(results) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! PQclear(results); ResetCancelConn(); goto sendquery_cleanup; } ! PQclear(results); transaction_status = PQtransactionStatus(pset.db); } --- 1104,1114 if (PQresultStatus(results) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! ClearOrSaveResult(results); ResetCancelConn(); goto sendquery_cleanup; } ! ClearOrSaveResult(results); transaction_status = PQtransactionStatus(pset.db); } *** SendQuery(const char *query) *** 1102,1112 if (PQresultStatus(results) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! PQclear(results); ResetCancelConn(); goto sendquery_cleanup; } ! PQclear(results); on_error_rollback_savepoint = true; } } --- 1129,1139 if (PQresultStatus(results) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! ClearOrSaveResult(results); ResetCancelConn(); goto sendquery_cleanup; } ! ClearOrSaveResult(results); on_error_rollback_savepoint = true; } } *** SendQuery(const char *query) *** 1202,1208 if (PQresultStatus(svptres) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! PQclear(svptres); OK = false; PQclear(results); --- 1229,1235 if (PQresultStatus(svptres) != PGRES_COMMAND_OK) { psql_error("%s", PQerrorMessage(pset.db)); ! ClearOrSaveResult(svptres); OK = false; PQclear(results); *** SendQuery(const char *query) *** 1213,1219 } } ! PQclear(results); /* Possible microtiming output */ if (pset.timing) --- 1240,1246 } } ! ClearOrSaveResult(results); /* Possible microtiming output */ if (pset.timing) *** ExecQuer
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
"Shulgin, Oleksandr" writes: > On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane wrote: >> Yeah, I don't much like that either. But I don't think we can avoid >> some refactoring there; as designed, conversion of an error message into >> user-visible form is too tightly tied to receipt of the message. > True. Attached is a v2 which addresses all of the points raised earlier I > believe. I took a closer look at what's going on here and realized that actually it's not that hard to decouple the message-building routine from the PGconn state, because mostly it works with fields it's extracting out of the PGresult anyway. The only piece of information that's lacking is conn->last_query. I propose therefore that instead of doing it like this, we copy last_query into error PGresults. This is strictly less added storage requirement than storing the whole verbose message would be, and it saves time compared to the v2 patch in the typical case where the application never does ask for an alternately-formatted error message. Plus we can actually support requests for any variant format, not only VERBOSE. Attached is a libpq-portion-only version of a patch doing it this way. I've not yet looked at the psql part of your patch. Comments? regards, tom lane diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 2328d8f..80f7014 100644 *** a/doc/src/sgml/libpq.sgml --- b/doc/src/sgml/libpq.sgml *** char *PQresultErrorMessage(const PGresul *** 2691,2696 --- 2691,2738 + + +PQresultVerboseErrorMessage + + PQresultVerboseErrorMessage + + + + + + Returns a reformatted version of the error message associated with + a PGresult object. + + char *PQresultVerboseErrorMessage(const PGresult *res, + PGVerbosity verbosity, + PGContextVisibility show_context); + + In some situations a client might wish to obtain a more detailed + version of a previously-reported error. + PQresultVerboseErrorMessage addresses this need + by computing the message that would have been produced + by PQresultErrorMessage if the specified + verbosity settings had been in effect for the connection when the + given PGresult was generated. If + the PGresult is not an error result, + PGresult is not an error result is reported instead. + The returned string includes a trailing newline. + + + + Unlike most other functions for extracting data from + a PGresult, the result of this function is a freshly + allocated string. The caller must free it + using PQfreemem() when the string is no longer needed. + + + + A NULL return is possible if there is insufficient memory. + + + + PQresultErrorFieldPQresultErrorField *** PGVerbosity PQsetErrorVerbosity(PGconn * *** 5582,5587 --- 5624,5631 mode includes all available fields. Changing the verbosity does not affect the messages available from already-existing PGresult objects, only subsequently-created ones. + (But see PQresultVerboseErrorMessage if you + want to print a previous error with a different verbosity.) *** PGContextVisibility PQsetErrorContextVis *** 5622,5627 --- 5666,5673 affect the messages available from already-existing PGresult objects, only subsequently-created ones. + (But see PQresultVerboseErrorMessage if you + want to print a previous error with a different display mode.) *** PQsetNoticeProcessor(PGconn *conn, *** 6089,6096 receiver function is called. It is passed the message in the form of a PGRES_NONFATAL_ERROR PGresult. (This allows the receiver to extract !individual fields using PQresultErrorField, or the complete !preformatted message using PQresultErrorMessage.) The same void pointer passed to PQsetNoticeReceiver is also passed. (This pointer can be used to access application-specific state if needed.) --- 6135,6143 receiver function is called. It is passed the message in the form of a PGRES_NONFATAL_ERROR PGresult. (This allows the receiver to extract !individual fields using PQresultErrorField, or complete !preformatted messages using PQresultErrorMessage or !PQresultVerboseErrorMessage.) The same void pointer passed to PQsetNoticeReceiver is also passed. (This pointer can be used to access application-specific state if needed.) diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index c69a4d5..21dd772 100644 *** a/src/interfaces/libpq/exports.txt --- b/src/interfaces/li
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Tue, Mar 15, 2016 at 4:44 PM, Shulgin, Oleksandr < oleksandr.shul...@zalando.de> wrote: > On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane wrote: > >> "Shulgin, Oleksandr" writes: >> > What I dislike about this POC is all the disruption in libpq, to be >> > honest. >> >> Yeah, I don't much like that either. But I don't think we can avoid >> some refactoring there; as designed, conversion of an error message into >> user-visible form is too tightly tied to receipt of the message. >> > > True. Attached is a v2 which addresses all of the points raised earlier I > believe. > > We still extract the message building part of code from > pqGetErrorNotice3(), but the user-facing change is much more sane now: just > added a new function PQresultVerboseErrorMessage(). > I wonder if this sort of change has any chance to be back-patched? If not, it would be really nice to have any sort of review soonish. Thank you. -- Alex
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane wrote: > "Shulgin, Oleksandr" writes: > > What I dislike about this POC is all the disruption in libpq, to be > > honest. > > Yeah, I don't much like that either. But I don't think we can avoid > some refactoring there; as designed, conversion of an error message into > user-visible form is too tightly tied to receipt of the message. > True. Attached is a v2 which addresses all of the points raised earlier I believe. We still extract the message building part of code from pqGetErrorNotice3(), but the user-facing change is much more sane now: just added a new function PQresultVerboseErrorMessage(). > It would be much neater if we could form the verbose message every > > time and let the client decide where to cut it. Maybe a bit "too clever" > > would be to put a \0 char between short message and it's verbose > > continuation. The client could then reach the verbose part like this > > (assuming that libpq did put a verbose part there): msg + strlen(msg) + > 1. > > Blech :-( > :-) That would not work either way, I've just noticed that SQLLEVEL is put at the start of the message in verbose mode, but not by default. Thinking about it, though, it seems to me that we could get away with > always performing both styles of conversion and sticking both strings > into the PGresult. That would eat a little more space but not much. > Then we just need to add API to let the application get at both forms. > This is what the v2 basically implements, now complete with help, tab-complete and documentation changes. I don't think we can add a usual simple regression test here reliably, due to LOCATION field which might be subject to unexpected line number changes. But do we really need one? -- Regards, Alex From d8d6a65da57d8e14eac4f614d31b19d52f8176d9 Mon Sep 17 00:00:00 2001 From: Oleksandr Shulgin Date: Wed, 6 Jan 2016 14:58:17 +0100 Subject: [PATCH] Add \errverbose psql command and support in libpq For every error we build both the default and verbose error message, then store both in PGresult. The client can then retrieve the verbose message using the new exported function PGresultVerboseErrorMessage(). In psql we store the verbose error message in pset (if any) for display when requested by the user with \errverbose. --- doc/src/sgml/libpq.sgml | 40 - src/bin/psql/command.c | 9 ++ src/bin/psql/common.c | 5 ++ src/bin/psql/help.c | 1 + src/bin/psql/settings.h | 2 + src/bin/psql/startup.c | 1 + src/bin/psql/tab-complete.c | 2 +- src/interfaces/libpq/exports.txt| 1 + src/interfaces/libpq/fe-exec.c | 29 ++- src/interfaces/libpq/fe-protocol3.c | 166 +--- src/interfaces/libpq/libpq-fe.h | 1 + src/interfaces/libpq/libpq-int.h| 3 +- 12 files changed, 181 insertions(+), 79 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 2328d8f..8b9544f 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2691,6 +2691,38 @@ char *PQresultErrorMessage(const PGresult *res); + + + PQresultVerboseErrorMessage + +PQresultVerboseErrorMessage + + + + + +Returns the most verbose error message associated with the +command, or an empty string if there was no error. + +char *PQresultVerboseErrorMessage(const PGresult *res); + +If there was an error, the returned string will include a trailing +newline. The caller should not free the result directly. It will +be freed when the associated PGresult handle is +passed to PQclear. + + + + If error verbosity is already set to the maximum available level, + this will return exactly the same error message + as PQresultErrorMessage. Otherwise it can be + useful to retrieve a more detailed error message without running + the failed query again (while increasing the error verbosity + level). + + + + PQresultErrorFieldPQresultErrorField @@ -5579,9 +5611,11 @@ PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity); this will normally fit on a single line. The default mode produces messages that include the above plus any detail, hint, or context fields (these might span multiple lines). The VERBOSE - mode includes all available fields. Changing the verbosity does not - affect the messages available from already-existing - PGresult objects, only subsequently-created ones. + mode includes all available fields. It is still possible to + retrieve the most verbose error message from an + existing PGresult object (without changing the + current verbosity level) + using PQresultVerboseErrorMessage. diff --git a/src/
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
"Shulgin, Oleksandr" writes: > What I dislike about this POC is all the disruption in libpq, to be > honest. Yeah, I don't much like that either. But I don't think we can avoid some refactoring there; as designed, conversion of an error message into user-visible form is too tightly tied to receipt of the message. > It would be much neater if we could form the verbose message every > time and let the client decide where to cut it. Maybe a bit "too clever" > would be to put a \0 char between short message and it's verbose > continuation. The client could then reach the verbose part like this > (assuming that libpq did put a verbose part there): msg + strlen(msg) + 1. Blech :-( Thinking about it, though, it seems to me that we could get away with always performing both styles of conversion and sticking both strings into the PGresult. That would eat a little more space but not much. Then we just need to add API to let the application get at both forms. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Wed, Feb 10, 2016 at 12:33 AM, Daniel Verite wrote: > Shulgin, Oleksandr wrote: > > > Most importantly, I'd like to learn of better options than storing the > > whole last_result in psql's pset structure. > > I guess that you could, each time a query fails, gather silently the > result of \errverbose, store it in a buffer, discard the PGresult, > and in case the user does \errverbose before running another query, > output what was in that buffer. > That's a neat idea. I also think that we could only store last PGresult when the query fails actually and discard it otherwise: the PGresult holding only the error doesn't occupy too much space. What I dislike about this POC is all the disruption in libpq, to be honest. It would be much neater if we could form the verbose message every time and let the client decide where to cut it. Maybe a bit "too clever" would be to put a \0 char between short message and it's verbose continuation. The client could then reach the verbose part like this (assuming that libpq did put a verbose part there): msg + strlen(msg) + 1. -- Alex
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Thu, Feb 11, 2016 at 10:53 AM, Pavel Stehule wrote: > > most recent error in verbose mode, without making a permanent session > >> > state change. Something like >> > >> > regression=# insert into bar values(1); >> > ERROR: insert or update on table "bar" violates foreign key constraint >> "bar_f1_fkey" >> > DETAIL: Key (f1)=(1) is not present in table "foo". >> > > > regression=# \saywhat > > Maybe its too cute but I'll give a +1 for this alone. David J.
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
> most recent error in verbose mode, without making a permanent session > > state change. Something like > > > > regression=# insert into bar values(1); > > ERROR: insert or update on table "bar" violates foreign key constraint > "bar_f1_fkey" > > DETAIL: Key (f1)=(1) is not present in table "foo". > > regression=# \saywhat > > ERROR: 23503: insert or update on table "bar" violates foreign key > constraint "bar_f1_fkey" > > DETAIL: Key (f1)=(1) is not present in table "foo". > > SCHEMA NAME: public > > TABLE NAME: bar > > CONSTRAINT NAME: bar_f1_fkey > > LOCATION: ri_ReportViolation, ri_triggers.c:3326 > > regression=# > > Wow, that's a fabulous idea. I see Oleksandr has tried to implement > it, although I haven't looked at the patch. But I think this would be > REALLY helpful. > yes +1 Pavel > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Tue, Jan 5, 2016 at 10:16 PM, Tom Lane wrote: > Jim Nasby writes: >> FWIW, I suspect very few people know about the verbosity setting (I >> didn't until a few months ago...) Maybe psql should hint about it the >> first time an error is reported in a session. > > Actually, what'd be really handy IMO is something to regurgitate the > most recent error in verbose mode, without making a permanent session > state change. Something like > > regression=# insert into bar values(1); > ERROR: insert or update on table "bar" violates foreign key constraint > "bar_f1_fkey" > DETAIL: Key (f1)=(1) is not present in table "foo". > regression=# \saywhat > ERROR: 23503: insert or update on table "bar" violates foreign key > constraint "bar_f1_fkey" > DETAIL: Key (f1)=(1) is not present in table "foo". > SCHEMA NAME: public > TABLE NAME: bar > CONSTRAINT NAME: bar_f1_fkey > LOCATION: ri_ReportViolation, ri_triggers.c:3326 > regression=# Wow, that's a fabulous idea. I see Oleksandr has tried to implement it, although I haven't looked at the patch. But I think this would be REALLY helpful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Shulgin, Oleksandr wrote: > Most importantly, I'd like to learn of better options than storing the > whole last_result in psql's pset structure. I guess that you could, each time a query fails, gather silently the result of \errverbose, store it in a buffer, discard the PGresult, and in case the user does \errverbose before running another query, output what was in that buffer. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Mon, Feb 8, 2016 at 5:24 PM, Daniel Verite 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 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 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
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
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 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-created ones." 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Wed, Jan 6, 2016 at 3:02 PM, Shulgin, Oleksandr < oleksandr.shul...@zalando.de> wrote: > > Please find attached a POC patch, using \errverbose for the command name. > Unfortunately, I didn't see a good way to contain the change in psql only > and had to change libpq, adding new interface PQresultBuildErrorMessage(). > Also, what I don't like very much is that we need to keep track of the last > result from last SendQuery() in psql's pset. So in my view this is sort of > a dirty hack that works nonetheless. > Added to the Open commitfest: https://commitfest.postgresql.org/9/475/
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On Wed, Jan 6, 2016 at 5:06 AM, Jim Nasby wrote: > On 1/5/16 9:16 PM, Tom Lane wrote: > >> Jim Nasby writes: >> >>> FWIW, I suspect very few people know about the verbosity setting (I >>> didn't until a few months ago...) Maybe psql should hint about it the >>> first time an error is reported in a session. >>> >> >> Actually, what'd be really handy IMO is something to regurgitate the >> most recent error in verbose mode, without making a permanent session >> state change. Something like >> >> regression=# insert into bar values(1); >> ERROR: insert or update on table "bar" violates foreign key constraint >> "bar_f1_fkey" >> DETAIL: Key (f1)=(1) is not present in table "foo". >> regression=# \saywhat >> ERROR: 23503: insert or update on table "bar" violates foreign key >> constraint "bar_f1_fkey" >> DETAIL: Key (f1)=(1) is not present in table "foo". >> SCHEMA NAME: public >> TABLE NAME: bar >> CONSTRAINT NAME: bar_f1_fkey >> LOCATION: ri_ReportViolation, ri_triggers.c:3326 >> regression=# >> >> Not sure how hard that would be to do within psql's current structure. >> > > At first glance, it looks like it just means changing AcceptResult() to > use PQresultErrorField in addition to PQresultErrorMessage, and stuffing > the results somewhere. And of course adding \saywhat to the psql parser, > but maybe someone more versed in psql code can verify that. > > If it is that simple, looks like another good beginner hacker task. :) Sorry, I couldn't resist it: I was too excited to learn such option existed. :-) Please find attached a POC patch, using \errverbose for the command name. Unfortunately, I didn't see a good way to contain the change in psql only and had to change libpq, adding new interface PQresultBuildErrorMessage(). Also, what I don't like very much is that we need to keep track of the last result from last SendQuery() in psql's pset. So in my view this is sort of a dirty hack that works nonetheless. Cheers! -- Alex From 402866a6899791e7f410ed747e3b0018eed717e0 Mon Sep 17 00:00:00 2001 From: Oleksandr Shulgin Date: Wed, 6 Jan 2016 14:58:17 +0100 Subject: [PATCH] POC: \errverbose in psql --- src/bin/psql/command.c | 20 ++ src/bin/psql/common.c | 6 +- src/bin/psql/settings.h | 1 + src/bin/psql/startup.c | 1 + src/interfaces/libpq/exports.txt| 1 + src/interfaces/libpq/fe-exec.c | 21 ++ src/interfaces/libpq/fe-protocol3.c | 131 +++- src/interfaces/libpq/libpq-fe.h | 1 + src/interfaces/libpq/libpq-int.h| 2 + 9 files changed, 122 insertions(+), 62 deletions(-) diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 9750a5b..9ae5ae5 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -822,6 +822,24 @@ exec_command(const char *cmd, } } + /* \errverbose -- display verbose error message from last result */ + else if (strcmp(cmd, "errverbose") == 0) + { + char *errmsg; + + if (pset.last_result && PQresultStatus(pset.last_result) == PGRES_FATAL_ERROR) + { + /* increase error verbosity for a moment */ + PQsetErrorVerbosity(pset.db, PQERRORS_VERBOSE); + errmsg = PQresultBuildErrorMessage(pset.db, pset.last_result); + PQsetErrorVerbosity(pset.db, pset.verbosity); + + psql_error("%s", errmsg); + + PQfreemem(errmsg); + } + } + /* \f -- change field separator */ else if (strcmp(cmd, "f") == 0) { @@ -1865,6 +1883,8 @@ do_connect(char *dbname, char *user, char *host, char *port) { PQfinish(o_conn); pset.db = NULL; +PQclear(pset.last_result); +pset.last_result = NULL; } } diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 2cb2e9b..85658e6 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -315,6 +315,8 @@ CheckConnection(void) psql_error("Failed.\n"); PQfinish(pset.db); pset.db = NULL; + PQclear(pset.last_result); + pset.last_result = NULL; ResetCancelConn(); UnsyncVariables(); } @@ -1154,7 +1156,9 @@ SendQuery(const char *query) } } - PQclear(results); + /* XXX: can we have PQclear that would free everything except errfields? */ + PQclear(pset.last_result); + pset.last_result = results; /* Possible microtiming output */ if (pset.timing) diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h index 20a6470..8b88fbb 100644 --- a/src/bin/psql/settings.h +++ b/src/bin/psql/settings.h @@ -80,6 +80,7 @@ enum trivalue typedef struct _psqlSettings { PGconn *db;/* connection to backend */ + PGresult *last_result; /* last result from running a query, if any */ int encoding; /* client_encoding */ FILE *queryFout; /* where to send the query results */ bool queryFoutPipe; /* queryFout is from a popen() */ diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 6916f6f..7586ee81 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -130,6 +130,7 @@ main(int argc, char *a
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On 1/5/16 9:16 PM, Tom Lane wrote: Jim Nasby writes: FWIW, I suspect very few people know about the verbosity setting (I didn't until a few months ago...) Maybe psql should hint about it the first time an error is reported in a session. Actually, what'd be really handy IMO is something to regurgitate the most recent error in verbose mode, without making a permanent session state change. Something like regression=# insert into bar values(1); ERROR: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". regression=# \saywhat ERROR: 23503: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". SCHEMA NAME: public TABLE NAME: bar CONSTRAINT NAME: bar_f1_fkey LOCATION: ri_ReportViolation, ri_triggers.c:3326 regression=# Not sure how hard that would be to do within psql's current structure. At first glance, it looks like it just means changing AcceptResult() to use PQresultErrorField in addition to PQresultErrorMessage, and stuffing the results somewhere. And of course adding \saywhat to the psql parser, but maybe someone more versed in psql code can verify that. If it is that simple, looks like another good beginner hacker task. :) -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Jim Nasby writes: > FWIW, I suspect very few people know about the verbosity setting (I > didn't until a few months ago...) Maybe psql should hint about it the > first time an error is reported in a session. Actually, what'd be really handy IMO is something to regurgitate the most recent error in verbose mode, without making a permanent session state change. Something like regression=# insert into bar values(1); ERROR: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". regression=# \saywhat ERROR: 23503: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". SCHEMA NAME: public TABLE NAME: bar CONSTRAINT NAME: bar_f1_fkey LOCATION: ri_ReportViolation, ri_triggers.c:3326 regression=# Not sure how hard that would be to do within psql's current structure. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On 1/5/16 8:41 PM, Tom Lane wrote: Jim Nasby writes: >does psql do anything with those fields? ISTM the biggest use for this >info is someone sitting at psql or pgAdmin. Sure, if you turn up the error verbosity. FWIW, I suspect very few people know about the verbosity setting (I didn't until a few months ago...) Maybe psql should hint about it the first time an error is reported in a session. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Jim Nasby writes: > does psql do anything with those fields? ISTM the biggest use for this > info is someone sitting at psql or pgAdmin. Sure, if you turn up the error verbosity. regression=# create table foo (f1 int primary key); CREATE TABLE regression=# create table bar (f1 int references foo); CREATE TABLE regression=# insert into bar values(1); ERROR: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". regression=# \set VERBOSITY verbose regression=# insert into bar values(1); ERROR: 23503: insert or update on table "bar" violates foreign key constraint "bar_f1_fkey" DETAIL: Key (f1)=(1) is not present in table "foo". SCHEMA NAME: public TABLE NAME: bar CONSTRAINT NAME: bar_f1_fkey LOCATION: ri_ReportViolation, ri_triggers.c:3326 I can't speak to pgAdmin, but if it doesn't make this info available the answer is to fix pgAdmin ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
On 1/5/16 7:21 PM, Tom Lane wrote: What seems more likely to be useful and to not break anything is to push forward on adding PG_DIAG_SCHEMA_NAME and similar auxiliary fields to more error messages (see errtable() and siblings). That would allow applications to extract this information automatically and reliably. Only after that work is complete and there's been a reasonable amount of time for clients to start relying on it can we really start thinking about whacking the message texts around. +1, but... does psql do anything with those fields? ISTM the biggest use for this info is someone sitting at psql or pgAdmin. Maybe schema info could be presented in HINT or DETAIL messages as well? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add schema-qualified relnames in constraint error messages.
Petr Korobeinikov writes: > At the moment schemas used as single-level namespaces. > It's quite convenient in large databases. > But error messages doesnât contain schema names. > I have done a little patch with schema-qualified relation names in error > messages that produced by foreign key constraints and triggers. We have gone around on this before and pretty much concluded we didn't want to do it; the demand is too small and the risk of breaking things too large. In particular, your patch as presented *would* break every application that is still parsing primary error messages to extract object names from them. What seems more likely to be useful and to not break anything is to push forward on adding PG_DIAG_SCHEMA_NAME and similar auxiliary fields to more error messages (see errtable() and siblings). That would allow applications to extract this information automatically and reliably. Only after that work is complete and there's been a reasonable amount of time for clients to start relying on it can we really start thinking about whacking the message texts around. Aside from those points, it's quite unacceptable to format qualified names as you propose here; they would really have to look more like "foo"."bar" to prevent confusion. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Add schema-qualified relnames in constraint error messages.
Hackers, At the moment schemas used as single-level namespaces. It's quite convenient in large databases. But error messages doesn’t contain schema names. I have done a little patch with schema-qualified relation names in error messages that produced by foreign key constraints and triggers. Patch and usage example are attached. Based on real bug hunting. Pre-reviewed by Alexander Korotkov. diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index b476500..997e959 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -343,7 +343,7 @@ RI_FKey_check(TriggerData *trigdata) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", - RelationGetRelationName(fk_rel), + RelationGetNamespaceQualifiedRelationName(fk_rel), NameStr(riinfo->conname)), errdetail("MATCH FULL does not allow mixing of null and nonnull key values."), errtableconstraint(fk_rel, @@ -2490,7 +2490,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) ereport(ERROR, (errcode(ERRCODE_FOREIGN_KEY_VIOLATION), errmsg("insert or update on table \"%s\" violates foreign key constraint \"%s\"", - RelationGetRelationName(fk_rel), + RelationGetNamespaceQualifiedRelationName(fk_rel), NameStr(fake_riinfo.conname)), errdetail("MATCH FULL does not allow mixing of null and nonnull key values."), errtableconstraint(fk_rel, @@ -2767,7 +2767,7 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation trig_rel, bool rel_is_pk) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("no pg_constraint entry for trigger \"%s\" on table \"%s\"", -trigger->tgname, RelationGetRelationName(trig_rel)), +trigger->tgname, RelationGetNamespaceQualifiedRelationName(trig_rel)), errhint("Remove this referential integrity trigger and its mates, then do ALTER TABLE ADD CONSTRAINT."))); /* Find or create a hashtable entry for the constraint */ @@ -2779,14 +2779,14 @@ ri_FetchConstraintInfo(Trigger *trigger, Relation trig_rel, bool rel_is_pk) if (riinfo->fk_relid != trigger->tgconstrrelid || riinfo->pk_relid != RelationGetRelid(trig_rel)) elog(ERROR, "wrong pg_constraint entry for trigger \"%s\" on table \"%s\"", -trigger->tgname, RelationGetRelationName(trig_rel)); +trigger->tgname, RelationGetNamespaceQualifiedRelationName(trig_rel)); } else { if (riinfo->fk_relid != RelationGetRelid(trig_rel) || riinfo->pk_relid != trigger->tgconstrrelid) elog(ERROR, "wrong pg_constraint entry for trigger \"%s\" on table \"%s\"", -trigger->tgname, RelationGetRelationName(trig_rel)); +trigger->tgname, RelationGetNamespaceQualifiedRelationName(trig_rel)); } return riinfo; @@ -3225,9 +3225,9 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo, ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("referential integrity query on \"%s\" from constraint \"%s\" on \"%s\" gave unexpected result", - RelationGetRelationName(pk_rel), + RelationGetNamespaceQualifiedRelationName(pk_rel), NameStr(riinfo->conname), - RelationGetRelationName(fk_rel)), + RelationGetNamespaceQualifiedRelationName(fk_rel)), errhint("This is most likely due to a rule having rewritten the query."))); /* @@ -3315,28 +3315,28 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,