Re: patch for 9.2: enhanced errors - Mailing list pgsql-hackers

On 11-06-08 04:14 PM, Pavel Stehule wrote:
> Hello
>
> Attached patch implements a new erros's fields that describes table,
> colums related to error. This enhanced info is limited to constraints
> and RI.
>

Here is my review of this patch

Submission Review:
------------------------
The patch applies cleanly against master
The patch does not include any documentation updates (see note below to 
update config.sgml)
The patch does not include any unit tests. At a minimum it should add a 
few tests with verbosity set to verbose


Usability Review
--------------------
The patch adds the ability to get more information about the reasons a 
query failed.  Pavel indicates that this is a building block for a later 
patch.   This sounds like a worthwhile goal, without this patch I don't 
see another good way of getting the details on what columns make up the 
constraint that fails, other than making additional queries into the 
catalog.

This patch should not impact pg_dump or pg_upgrade.

Pavel has submitted a related patch that adds support for this feature 
to plpgsql,  in theory other pl's might want to use the information this 
patch exposes.


Feature Test
-------------------

The error messages behave as described with \set verbosity verbose.

I tried this using both the 8.3 and 9.0 versions of psql (against a 
postgresql server with this patch) and things worked as expected (the 
extra error messages did not show).  I also tried the patched psql 
against an 8.3 backend and verified that we don't get strange behaviour 
going against an older backend with verbosity set.

I tried adding multiple constraints to a table and inserting a row that 
violates them, only one of the constraints showed up in the error 
message, this is fine and consistent with the existing behaviour


Consider this example of an error that gets generated

ERROR:  23505: duplicate key value violates unique constraint "A_pkey"
DETAIL:  Key (a)=(1) already exists.
LOCATION:  _bt_check_unique, nbtinsert.c:433
CONSTRAINT:  A_pkey
SCHEMA:  public
TABLE:  A
COLUMN:  a
STATEMENT:  insert into "A" values (1);

I think that both the CONSTRAINT, and TABLE name should be double quoted 
like "A_pkey" is above.  When doing this make sure you don't break the 
quoting in the CSV format log.


Performance Review
-----------------------------
I don't see this patch impacting performance, I did not conduct any 
performance tests.


Coding Review
-----------------


In tupdesc.c

line 202 the existing code is performing a deep copy of ConstrCheck.  Do 
you need to copy nkeys and conkey here as well?

Then at line 250 ccname is freed but not conkey


postgres_ext.h line 55
+ #define PG_DIAG_SCHEMA_NAME    's'
+ #define PG_DIAG_TABLE_NAME    't'
+ #define PG_DIAG_COLUMN_NAMES    'c'
+ #define PG_DIAG_CONSTRAINT_NAME 'n'

The assignment of letters to parameters seems arbitrary to me, I don't 
have a different non-arbitrary mapping in mind but if anyone else does 
they should speak up.  I think it will be difficult to change this after 
9.2 goes out.


elog.c:
***************
*** 2197,2202 ****
--- 2299,2319 ----      if (application_name)          appendCSVLiteral(&buf, application_name);

+     /* constraint_name */
+     appendCSVLiteral(&buf, edata->constraint_name);
+     appendStringInfoChar(&buf, ',');
+
+     /* schema name */
+     appendCSVLiteral(&buf, edata->schema_name);
+     appendStringInfoChar(&buf, ',');

You need to update config.sgml at the same time you update this format.
You need to append a "," after application name but before 
constraintName.   As it stands the CSV log has something like:
.....nbtinsert.c:433","psql""a_pkey","public","a","a"


nbtinsert.c

pg_get_indrelation is named differently than everything else in this 
file (ie _bt...).  My guess is that this function belongs somewhere else 
but I don't know the code well enough to say where you should move it too.



Everything I've mentioned above is a minor issue, I will move the patch 
to 'waiting for author' and wait for you to release an updated patch.

Steve Singer