Thread

Re: patch for 9.2: enhanced errors

Pavel Stehule <pavel.stehule@gmail.com> — 2011-06-20T19:44:20Z

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