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

Hello

2011/6/19 Steve Singer <ssinger_pg@sympatico.ca>:
> 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.
>>
> ...
>
> 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.
>

I agree so quoting must be used in CSV log - the result have to be
valid CSV and I'll ensure this. I am not sure about implicit quoting
and using some quote_ident operation early. This is result of some
operation - not input. Quoting in message is used not like SQL
quoting, but as plain text quoting - it is just border between human
readable text and data. But fields like TABLE_NAME or COLUMN_NAME
contains just data - so quoting is useless.

Next argument - the quoting is more simple than remove quoting. If
somebody needs to quoting, then can use a quoting_ident function, but
there are no inverse function - so I prefer a names in raw format. It
is more simply and usual to add quoting than remove quoting.

What do you think about?


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

I have to look on this

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

ok

>
> 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'll try to get better name, but I would not use a public name like _bt

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

ok

Thank you very much

Pavel Stehule