Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Alex Shulgin]

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.

[Tom Lane]

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.

[Tom Lane]

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.

[Alex Shulgin]

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.

[Peter Geoghegan]

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.

[Tom Lane]

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

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Tom Lane]

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

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Shulgin, Oleksandr]

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.

[Shulgin, Oleksandr]

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 

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Tom Lane]

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

[Shulgin, Oleksandr]

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.

[David G. Johnston]

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.

[Robert Haas]

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.

[Pavel Stehule]

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

[Shulgin, Oleksandr]

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.

[Daniel Verite]

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.

[Daniel Verite]

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.

[Shulgin, Oleksandr]

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.

[Shulgin, Oleksandr]

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

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Jim Nasby]

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.

[Tom Lane]

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.

[Jim Nasby]

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.

[Tom Lane]

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.

[Jim Nasby]

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.

[Tom Lane]

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