On 7 July 2012 13:57, Pavel Stehule <pavel.steh...@gmail.com> wrote: >> In my revision, I've just added a pre-declaration and removed the >> dedicated header, which didn't make too much sense to me: >> >> + /* Pre-declare Relation, in order to avoid a build dependency on rel.h. */ >> + typedef struct RelationData *Relation;
>> Opaque pointers are ordinarily used to encapsulate things in C, rather >> than to prevent build dependencies, but I believe that's only because >> in general that's something that C programmers are more likely to >> want. >> > > It is question for Alvaro or Tom. I have not strong opinion on it. Fair enough. >> You always log all of these new fields within write_csvlog(), even if >> (Log_error_verbosity < >> PGERROR_VERBOSE). Why? > it is bug - these new fields should be used only when verbosity is >= VERBOSE Please fix it. >> +#define PG_DIAG_TRIGGER_SCHEMA 'h' >> >> Not all appear to have a way of setting the value within the ereport >> interface. For example, there is nothing like "errrelation_column()" >> (or "errrelcol()", as I call it) to set PG_DIAG_ROUTINE_NAME. This is >> something I haven't touched. > > When I sent this patch first time, then one issue was new functions > for these fields. Tom proposal was using a generic function for these > new fields. These fields holds separated values, but in almost all > cases some combinations are used - "ROUTINE_NAME, ROUTINE_SCHEMA", > "TABLE_NAME, TABLE_SCHEMA" - so these fields are not independent - > this is difference from original ErrorData fields - so axillary > functions doesn't follow these fields - because it is not practical. Maybe it isn't practical to do it that way, but I think that we need to have a way of setting the fields from an ereport callsite. I am willing to accept that it may make sense to add existing ereport sites by piecemeal, in later patches, but I think you should figure out how regular ereport sites are supposed to do this before anything is committed. We need to nail down the interface first. > I understand, but fixing any ereport in core is difficult for > processing. So coverage only some subset is practical (first stage) - > with some basic infrastructure in core all other patches with better > covering will be simpler for review and for commit too. RI and > constraints is more often use cases where you would to parse error > messages - these will be covered in first stage. Okay. What subset? I would hope that it was a well-defined subset. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers