Hello 2011/6/19 Steve Singer <ssinger...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
