Re: enhanced error fields

Hello

2012/12/10 Peter Geoghegan <peter@2ndquadrant.com>:
> So, I took a look at this, and made some more changes.
>
> I have a hard time seeing the utility of some fields that were in this
> patch, and so they have been removed from this revision.
>
> Consider, for example:
>
> +   constraint_table text,
> +   constraint_schema text,
>
> While constraint_name remains, I find it really hard to believe that
> applications need a perfectly unambiguous idea of what constraint
> they're dealing with. If applications have a constraint that has a
> different domain-specific meaning depending on what schema it is in,
> while a table in one schema uses that constraint in another, well,
> that's a fairly awful design. Is it something that we really need to
> care about? You mentioned the SQL standard, but as far as I know the
> SQL standard has nothing to say about these fields within something
> like errdata - their names are lifted from information_schema, but
> being unambiguous is hardly so critical here. We want to identify an
> object for the purposes of displaying an error message only. Caring
> deeply about ambiguity seems wrong-headed to me in this context.
>
> +   routine_name text,
> +   trigger_name text,
> +   trigger_table text,
> +   trigger_schema text,
>
> The whole point of an exception (which ereport() is very similar to)
> is to decouple errors from error handling as much as possible - any
> application that magically takes a different course of action based on
> where that error occurred as reported by the error itself (and not the
> location of where handling the error occurs) seems kind of
> wrong-headed to me. Sure, a backtrace may be supplied, but that's only
> for diagnostic purposes, and a human can just as easily get that
> information already. If you can present an example of this information
> actually being present in a way that is amenable to that, either in
> any other RDBMS or any major programming language or framework, I
> might reconsider.
>
> This just leaves:
>
> +   column_name text,
> +   table_name text,
> +   constraint_name text,
> +   schema_name text,

I agree so lot of mentioned fields are difficult defined in PostgreSQL
environment due different meaning between ANSI SQL and PostgreSQL and
because PostgreSQL doesn't support some ANSI SQL features -
assertions.

There are two basic aspects of design

* usability - we would to remove necessity of parsing error messages
for getting interesting data, it decrease dependency on current error
messages - then I am thinking so some informations about triggers or
functions where exception was born are practical (current
implementation is different task - we can find better solution than I
proposed highly probably)

* compatibility - personally, lot of diagnostics fields are very vague
defined, so here can be lot of issues - I have no experience with DB2
or Oracle, so I cannot to speak more, but we should to say, what we
expect.

>
> This seems like enough information for any reasonable use of this
> infrastructure, and I highly doubt anyone would avail of the other
> fields if they remained. I guess an application might have done
> something with "constraint_table", as with foreign keys for example,
> but I just can't see that being used when constraint_name can be used.
>

I afraid so constraint name is not afraid

postgres=# create table a (a int primary key);
CREATE TABLE
postgres=# create table b (b int references a(a));
CREATE TABLE
postgres=# insert into b values (10);
ERROR:  insert or update on table "b" violates foreign key constraint "b_b_fkey"
DETAIL:  Key (b)=(10) is not present in table "a".
postgres=# \set VERBOSITY verbose
postgres=# insert into b values (10);
ERROR:  23503: insert or update on table "b" violates foreign key
constraint "b_b_fkey"
DETAIL:  Key (b)=(10) is not present in table "a".
LOCATION:  ri_ReportViolation, ri_triggers.c:3222
postgres=# insert into a values(10);
INSERT 0 1
postgres=# insert into b values (10);
INSERT 0 1
postgres=# delete from a;
ERROR:  23503: update or delete on table "a" violates foreign key
constraint "b_b_fkey" on table "b"
DETAIL:  Key (a)=(10) is still referenced from table "b".
LOCATION:  ri_ReportViolation, ri_triggers.c:3232

there are two different bugs - with same ERRCODE and constraint name -
once is problem on "b", second on "a"

so constraint_table is interesting information

> Removing these fields has also allowed me to remove the "avoid setting
> field at lower point in the callstack" logic, which was itself kind of
> ugly. Since fields like routine_name only set themselves at the top of
> the callstack, the potential for astonishing outcomes was pretty high
> - what if you expect one routine_name, but then that routine follows a
> slightly different code-path one day and you get another function
> setting routine_name and undermining that expectation?
>

My implementation is probably too dumb - but a question - "where
exception is coming from?" is relative typical - but we can to clean
implementation. I see a issue in definition. Usually I am interesting
about most deep custom code - not most deep code.

so I am not against to removing some fields, but constraint_table and
routine_name is useful and we should to produce this information.


> There were some bugs left in the patch eelog3.diff, mostly due to
> things like setting table name to what is actually an index name. As I
> mentioned, we now assert that:
>
> +       Assert(table->rd_rel->relkind == RELKIND_RELATION);
>
> in the functions within relerror.c.
>
> I have tightened up where these fields are available, and
> appropriately documented that for the benefit of both application
> developers and developers of client libraries. I went so far as to
> hack the Perl scripts that generate .sgml and .h files from
> errcodes.txt (i.e. the infrastrucutre that produces tables of errcodes
> in various places from a single authoritative place) - I have
> instituted a coding standard so that these fields are reliably
> available and have documented that requirement at both the user and
> hacker level.
>
> It would be nice if a Perl hacker could eyeball those changes - this
> is my first time writing Perl, and I suspect it may be worth having
> someone else to polish the Perl code a bit.
>
> I have emphasized the need for consistency and a sane contract for
> application developers and third-party client driver authors - they
> *need* to know that certain fields will always be available, or at
> least won't be unavailable due to a change in the phase of the moon.
>
> errcodes.txt now says:
>
> + # Postgres coding standards mandate that certain fields be available in all
> + # instances for some of the Class 23 errcodes, documented under
> "Requirement: "
> + # here.  Some other errcode's ereport sites may, at their own discretion, make
> + # errcolumn, errtable, errconstraint and errschema fields available too.
> + # Furthermore, it is possible to make some fields available beyond those
> + # formally required at callsites involving these Class 23 errcodes with
> + # "Requirements: ".
>   Section: Class 23 - Integrity Constraint Violation
> ! Requirement: unused
>   23000    E    ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION
>         integrity_constraint_violation
> + Requirement: unused
>   23001    E    ERRCODE_RESTRICT_VIOLATION
>         restrict_violation
> + # Note that requirements for ERRCODE_NOT_NULL do not apply to domains:
> + Requirement: schema_name, table_name
>   23502    E    ERRCODE_NOT_NULL_VIOLATION
>         not_null_violation
> + Requirement: schema_name, table_name, constraint_name
>   23503    E    ERRCODE_FOREIGN_KEY_VIOLATION
>         foreign_key_violation
> + Requirement: schema_name, table_name, constraint_name
>   23505    E    ERRCODE_UNIQUE_VIOLATION
>         unique_violation
> + Requirement: constraint_name
>   23514    E    ERRCODE_CHECK_VIOLATION
>         check_violation
> + Requirement: schema_name, table_name, constraint_name
>   23P01    E    ERRCODE_EXCLUSION_VIOLATION
>         exclusion_violation
>
> Now, there are one or two places where these fields are not actually
> available even though they're formally required according to a literal
> reading of the above. This is only because there is clearly no such
> field sensibly available, even in principle - to my mind this cannot
> be a problem, because the application developer cannot have any
> reasonable expectation of a field being set. I'm really talking about
> two cases in particular:
>
> * For ERRCODE_NOT_NULL_VIOLATION, we don't actually provide
> schema_name and table_name in the event of domains. This was
> previously identified as an issue. If it is judged better to not have
> any requirements there at all, so be it.
>
> * For the validateDomainConstraint() ERRCODE_CHECK_VIOLATION ereport
> call, we may not provide a constraint name iff a Constraint.connname
> is NULL. Since there isn't a constraint name to give even in
> principle, and this is an isolated case, this seems reasonable.

ok

>
> To make logging less verbose, TABLE NAME isn't consistently split out
> as a separate field - this seems fine to me, since application code
> doesn't target logs:
>
> +                       if (edata->column_name && edata->table_name)
> +                       {
> +                               log_line_prefix(&buf, edata);
> +                               appendStringInfo(&buf, _("COLUMN NAME:  %s:%s\n"),
> +                                                                edata->table_name, edata->column_name);
> +                       }
> +                       else if (edata->table_name)
> +                       {
> +                               log_line_prefix(&buf, edata);
> +                               appendStringInfo(&buf, _("TABLE NAME:  %s\n"),
> +                                                                edata->table_name);
> +                       }

uff, no - I don't like it - it is not consistent and I don't think so
one row more is a issue. But is a question if we don't need a second
"VERBOSE" level for showing these fields - maybe VERBOSE_ENHANCED or
some similar

we don't need log these fields usually ??

>
> I used pgindent to selectively indent parts of the codebase affected
> by this patch. I am about ready to mark this one "ready for
> committer", but it would be nice at this point to get some buy-in on
> the basic view of how these things should work that informed this
> revision. Does anyone disagree with my contention that there should be
> a sane, well-defined contract, or any of the details of what that
> should look like? Was I right to suggest that some of the set of
> fields that appeared in Pavel's eelog3.diff revision are unnecessary?
>
> I'm sorry it took me as long as it did to get back to you on this.
>

It is ok, thank you very much

Regards

Pavel

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