Re: patch for 9.2: enhanced errors

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