Hello
I am sending a updated patch
>
> 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
>
fixed
>
> 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"
fixed
>
>
> 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.
>
I renamed this function to IndexRelationGetParentRelation and muved to
relcache.c
I don't call a quote_identifier on only data error properties like
table_name or schema_name (but I am open to arguments for it or
against it). The quote_identifier is used for column names, because
there should be a more names and comma should be used inside name -
and this is consistent with pg_get_indexdef_columns.
Regards
Pavel Stehule
