Add schema-qualified relnames in constraint error messages.

Started by Petr Korobeinikovover 10 years ago26 messageshackers
Jump to latest
#1Petr Korobeinikov
pkorobeinikov@gmail.com

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.

Attachments:

namespaced-relname.difftext/plain; charset=US-ASCII; name=namespaced-relname.diffDownload+159-142
example.sqlapplication/octet-stream; name=example.sqlDownload
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Petr Korobeinikov (#1)
Re: Add schema-qualified relnames in constraint error messages.

Petr Korobeinikov <pkorobeinikov@gmail.com> 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

#3Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Tom Lane (#2)
Re: 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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jim Nasby (#3)
Re: Add schema-qualified relnames in constraint error messages.

Jim Nasby <Jim.Nasby@BlueTreble.com> 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

#5Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Tom Lane (#4)
Re: Add schema-qualified relnames in constraint error messages.

On 1/5/16 8:41 PM, Tom Lane wrote:

Jim Nasby<Jim.Nasby@BlueTreble.com> 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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jim Nasby (#5)
Re: Add schema-qualified relnames in constraint error messages.

Jim Nasby <Jim.Nasby@bluetreble.com> 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

#7Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Tom Lane (#6)
Re: Add schema-qualified relnames in constraint error messages.

On 1/5/16 9:16 PM, Tom Lane wrote:

Jim Nasby <Jim.Nasby@bluetreble.com> 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

#8Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Jim Nasby (#7)
Re: Add schema-qualified relnames in constraint error messages.

On Wed, Jan 6, 2016 at 5:06 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

On 1/5/16 9:16 PM, Tom Lane wrote:

Jim Nasby <Jim.Nasby@bluetreble.com> 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

Attachments:

0001-POC-errverbose-in-psql.patchtext/x-patch; charset=US-ASCII; name=0001-POC-errverbose-in-psql.patchDownload+122-63
#9Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Shulgin, Oleksandr (#8)
Re: Add schema-qualified relnames in constraint error messages.

On Wed, Jan 6, 2016 at 3:02 PM, Shulgin, Oleksandr <
oleksandr.shulgin@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/

#10Daniel Verite
daniel@manitou-mail.org
In reply to: Shulgin, Oleksandr (#9)
Re: 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 <const PGresult*> as input.
It's not clear why PGconn* is necessary.

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

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

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

- a description of the function should be added to the SGML doc.
There's a sentence in PQsetErrorVerbosity() that says, currently:

"Changing the verbosity does not affect the messages available from
already-existing PGresult objects, only subsequently-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

#11Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Daniel Verite (#10)
Re: Add schema-qualified relnames in constraint error messages.

On Mon, Feb 8, 2016 at 5:24 PM, Daniel Verite <daniel@manitou-mail.org>
wrote:

Shulgin, Oleksandr wrote:

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

Here's a review. Note that the patch tested and submitted
is not the initial one in the thread, so it doesn't exactly
match $subject now.

I've edited the CF entry title to read: Add \errverbose to psql (and
support in libpq)

What's tested here is a client-side approach, suggested by Tom

upthread as an alternative, and implemented by Oleksandr in
0001-POC-errverbose-in-psql.patch

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

psql part
=========
Compiles and works as intended.
For instance with \set VERBOSITY default, an FK violation produces:

# insert into table2 values(10);
ERROR: insert or update on table "table2" violates foreign key
constraint
"table2_id_tbl1_fkey"
DETAIL: Key (id_tbl1)=(10) is not present in table "table1".

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

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

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

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

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

My comments:

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

Not exactly, the verbosity setting might or might not be in effect when
this function is called. Another external part of the state that might
affect the result is conn->show_context field.

I would expect the new function to have the same interface than

PQresultErrorMessage(), but it's not the case.

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

This is because PQresultErrorMessage() returns a stored error message:
res->errMsg (or "").

- it returns a pointer to a strdup'ed() buffer, which

has to be freed separately by the caller. That differs from
PQresultErrorMessage() which keeps this managed inside the
PGresult struct.

For the same reason: this function actually produces a new error message,
as opposed to returning a stored one.

- if protocol version < 3, an error message is returned. It's not

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

Hm, probably we could just copy the message from conn->errorMessage, in
case of protocol v2, but I don't see a point in passing PGresult to the
func or naming it PQresult<Something> in the case of v2.

- if the fixed error message is kept, it should pass through

libpq_gettext() for translation.

Good point.

- a description of the function should be added to the SGML doc.

There's a sentence in PQsetErrorVerbosity() that says, currently:

"Changing the verbosity does not affect the messages available from
already-existing PGresult objects, only subsequently-created ones."

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

I didn't touch the documentation specifically, because this is just a
proof-of-concept, but it's good that you notice this detail. Most
importantly, I'd like to learn of better options than storing the whole
last_result in psql's pset structure.

Thanks for the review!

--
Alex

#12Daniel Verite
daniel@manitou-mail.org
In reply to: Shulgin, Oleksandr (#11)
Re: 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

#13Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#6)
Re: Add schema-qualified relnames in constraint error messages.

On Tue, Jan 5, 2016 at 10:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Jim Nasby <Jim.Nasby@bluetreble.com> 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

#14Pavel Stehule
pavel.stehule@gmail.com
In reply to: Robert Haas (#13)
Re: 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

Show quoted text

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

#15David G. Johnston
david.g.johnston@gmail.com
In reply to: Pavel Stehule (#14)
Re: Add schema-qualified relnames in constraint error messages.

On Thu, Feb 11, 2016 at 10:53 AM, Pavel Stehule <pavel.stehule@gmail.com>
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.

#16Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Daniel Verite (#12)
Re: Add schema-qualified relnames in constraint error messages.

On Wed, Feb 10, 2016 at 12:33 AM, Daniel Verite <daniel@manitou-mail.org>
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

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Shulgin, Oleksandr (#16)
Re: Add schema-qualified relnames in constraint error messages.

"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> 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

#18Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Tom Lane (#17)
Re: Add schema-qualified relnames in constraint error messages.

On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> 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

Attachments:

errverbose-psql-v2.patchtext/x-patch; charset=US-ASCII; name=errverbose-psql-v2.patchDownload+181-80
#19Shulgin, Oleksandr
oleksandr.shulgin@zalando.de
In reply to: Shulgin, Oleksandr (#18)
Re: Add schema-qualified relnames in constraint error messages.

On Tue, Mar 15, 2016 at 4:44 PM, Shulgin, Oleksandr <
oleksandr.shulgin@zalando.de> wrote:

On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> 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

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Shulgin, Oleksandr (#18)
Re: Add schema-qualified relnames in constraint error messages.

"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:

On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

Attachments:

errverbose-v3.patchtext/x-diff; charset=us-ascii; name=errverbose-v3.patchDownload+275-113
#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#20)
In reply to: Robert Haas (#13)
#23Alex Shulgin
alex.shulgin@gmail.com
In reply to: Tom Lane (#20)
#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alex Shulgin (#23)
#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Geoghegan (#22)
#26Alex Shulgin
alex.shulgin@gmail.com
In reply to: Tom Lane (#25)