Re: enhanced error fields

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(dot)stehule(at)gmail(dot)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.

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

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.

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

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