Re: enhanced error fields

On 26 January 2013 22:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I started to look this patch over.  I think we can get to something
> committable from here, but I'm having a problem with the concept that
> we're going to "guarantee" anything about which additional fields might
> be available for any given SQLSTATE.  This is already quite broken for
> the ERRCODE_NOT_NULL_VIOLATION case, and it's not hard to envision that
> there will be other inconsistencies in future, even without the issue
> that third-party code might use these SQLSTATEs without having gotten
> the memo about additional fields.

> I'm inclined to remove the "requirements" business altogether and just
> document that these fields may be supplied, or words to that effect.
> In practice, an application is going to know whether the particular
> error case it's concerned about has the auxiliary fields, I should think.

I think we may be talking at cross purposes here. Guarantee may have
been too strong a word, or the wrong word entirely. All that I really
want here is for there to be a coding standard instituted, so that in
future client code will not be broken by a failure to include some
field in a new ereport site that related to what is effectively the
same error as an existing ereport site. I'm sure you'll agree that
subtly breaking client code when refactoring Postgres is unacceptable.
I thought that the best way to do that was at the errcode granularity,
and am perfectly willing to pull back on the set of fields where this
coding standard applies in respect of certain errcodes. I think we can
afford to be very conservative about limiting the scope of that
guarantee.

>> We also go to extra lengths to get a table_name for certain
>> domain-related ereport sites.
>
> A lot of that looks pretty broken to me, eg the changes in
> ExecEvalCoerceToDomain are just hokum.  (Even if the expression is
> executing in a statement that has a result table, there's no very good
> reason to think that the error has anything to do with the result
> table.)

I must admit that that particular change wasn't very well thought out.
I was trying to appease Stephen, who seemed to think that having a
table_name where it was in principle available was particularly
important. I myself do not, and the current structure of the relevant
code (and difficulty in providing a table_name consistently) in a
sense reflects that. I'm inclined to agree that it is not worth
pursuing further.

> BTW, one thing that struck me in a quick look-through is that the
> ERRCODE_FOREIGN_KEY_VIOLATION patches seem to inconsistently send
> either the PK or FK rel as the "errtable".  Is this really per spec?
> I'd have sort of expected that the reported table ought to be the one
> that the constraint belongs to, namely the FK table.

Personally, on the face of it I'd expect the "inconsistency" to simply
reflect the fact that the error related to the referencing table or
referenced table. Pavel's original patch followed the same convention
(though it also had a constraint_table field). I'm having a hard time
figuring out the standards intent here, and I'm not sure that we
should even care, because that applies on to GET DIAGNOSTICS, which
isn't really the same thing as what we have here. I defer to you,
though - it's not as if I feel too strongly about it.

As I've said, the vast majority of the value likely to be delivered by
this patch comes from the constraint_name field. That's the really
compelling one, to my mind.

-- 
Regards,
Peter Geoghegan