Re: enhanced error fields

Peter Geoghegan <peter(dot)geoghegan86(at)gmail(dot)com> writes:
> On 26 January 2013 22:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm inclined to remove the "requirements" business altogether and just
>> document that these fields may be supplied, or words to that effect.

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

[ shrug... ] If you have a way of making a guarantee that future
versions introduce no new bugs, patent it and you'll soon have all the
money in the world.

It's conceivable that we could adapt some static checker to look for
ereport calls that mention particular ERRCODEs and lack particular
helper functions, but even that wouldn't be a cast-iron guarantee
because of the possibility of call sites using non-constant errcode
values. It'd probably be good enough in practice though. However,
this patch is not that, and mere documentation isn't going to buy a
thing here IMO. Especially not user-facing documentation, as opposed
to something that might be in a developers' face when he's
copying-and-pasting code somewhere. This patch didn't even touch the
one place in the documentation that might be somewhat useful from a
developer's standpoint, which is 49.2. Reporting Errors Within the
Server.

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

Well, if we can get an accurate and relevant value then I'm all for
adding it. But field values that are misleading or even plain wrong
in corner cases are not an improvement.

At some point we might want to undertake a round of refactoring that
makes this type of information available; and once we do, we'd probably
want to expose it in the regular message texts not just the auxiliary
fields. I think that sort of work is out-of-scope for this patch
though. IMO what we should be doing here is getting the infrastructure
in place, and then decorating some basic set of messages with aux info
in places where not a lot of new code is needed to make that happen.
Extending the decoration beyond that is material for future work.

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

A large part of the argument for doing this patch at all is to satisfy
the standard's requirements for information reported to a client.
(I believe that GET DIAGNOSTICS is in the end a client-side requirement,
ie in principle ecpg or similar library should be able to implement
it based on what the server reports.) So to the extent that the spec
defines what should be in the fields, we need to follow that.

regards, tom lane