Re: enhanced error fields

On 7 July 2012 13:57, Pavel Stehule <pavel.stehule@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