Re: enhanced error fields

2012/7/7 Peter Geoghegan <peter@2ndquadrant.com>:
> Attached is a revision of this patch, with a few clean-ups, mostly to
> the wording of certain things.
>
> On 5 July 2012 17:41, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> * renamed auxiliary functions and moved it elog.c - header is new file
>> "relerror.h"
>
> 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;
>
> This works fine, and is precedented. See
> src/include/storage/buffile.h, for example. If you think it's
> unreasonable that none of the functions now added to elog.h are
> callable without also including rel.h, consider that all call sites
> are naturally already doing that, for the errmsg() string itself.
> Besides, this is a perfectly valid way of reducing build dependencies,
> or at least a more valid way than creating a new header that does not
> really represent a natural new division for these new functions, IMHO.
> 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.

>> * new fields "constraint_table" and "trigger_table" - constraints and
>> triggers are related to relation in pg, not just to schema
>
> I've added some remarks to that effect in the docs of my revision for
> your consideration.
>
>> * better coverage of enhancing errors in source code
>
> Good. I think it's important that we nail down just where these are
> expected to be available. It would be nice if there was a quick and
> easy answer to the question "Just what ErrorResponse fields should
> this new "sub-category of class 23" ereport() site have?".  We clearly
> have yet to work those details out.
>
> I have another concern with this patch. log_error_verbosity appears to
> be intended as an exact analogue of client verbosity (as set by
> PQsetErrorVerbosity()). While this generally holds for this patch,
> there is one notable exception: You always log all of these new fields
> within write_csvlog(), even if (Log_error_verbosity <
> PGERROR_VERBOSE). Why? There will be a bunch of commas in most CSV
> logs once this happens, so that the schema of the log is consistent.
> That is kind of ugly, but I don't see a way around it. We already do
> this for location and application_name (though that's separately
> controlled by the application_name guc). I haven't touched that in the
> attached revision, as I'd like to hear what you have to say.

it is bug - these new fields should be used only when verbosity is >= VERBOSE

>
> Another problem that will need to be fixed is that of the follow values:
>
> +#define PG_DIAG_COLUMN_NAME    'c'
> +#define PG_DIAG_TABLE_NAME     't'
> +#define PG_DIAG_SCHEMA_NAME    's'
> +#define PG_DIAG_CONSTRAINT_NAME                'n'
> +#define PG_DIAG_CONSTRAINT_TABLE       'o'
> +#define PG_DIAG_CONSTRAINT_SCHEMA      'm'
> +#define PG_DIAG_ROUTINE_NAME           'r'
> +#define PG_DIAG_ROUTINE_SCHEMA         'u'
> +#define PG_DIAG_TRIGGER_NAME           'g'
> +#define PG_DIAG_TRIGGER_TABLE          'i'
> +#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.

>
>>> src/backend/utils/adt/domains.c
>>> 162:                                                            (errcode(ERRCODE_CHECK_VIOLATION),
>>>
>>
>> these exceptions are related to domains - we has not adequate fields
>> now - and these fields are not in standards
>>
>> it needs some like DOMAIN_NAME and DOMAIN_SCHEMA ???
>
> Hmm. I'm not sure that it's worth it.
>
> I took a look at recent JDBC documentation, because I'd expect it to
> offer the most complete support for exposing this in exception
> classes. Turns out that it does not expose things at as fine a
> granularity as you have here at all, which is disappointing. It seems
> to suppose that just a vendor code and "cause" will be sufficient. If
> you're using this one proprietary database, there is a command that'll
> let you get diagnostics. The wisdom for users of another proprietary
> database seems to be that you just use stored procedures. So I agree
> that CONSTRAINT NAME should be unset in the event of uncatalogued,
> unnamed not null integrity constraint violations. The standard seems
> to be loose on this, and I think we'll have to be too. However, I'd
> find it pretty intolerable if we were inconsistent between ereport()
> callsites that were *effectively the same error* - this could result
> in a user's application breaking based on the phase of the moon.
>
> Do you suppose it's worth stashing the last set of these fields to one
> side, and exposing this through an SQL utility command, or even a
> bundled function? I don't imagine that it's essential to have that
> right away, but it's something to consider.

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.

Regards

Pavel

>
> --
> Peter Geoghegan       http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training and Services