Re: Small foreign key error message improvement

I recently had a puzzler, which involved this sort of accidental parser error:

CREATE TABLE foo (a int, b text, PRIMARY KEY (a, b));

CREATE TABLE bar (x int, y text, FOREIGN KEY (q, r) REFERENCES foo (m, n));
ERROR: column "q" referenced in foreign key constraint does not exist

versus

CREATE TABLE bar (x int, y text, FOREIGN KEY (x, y) REFERENCES foo (m, n));
ERROR: column "m" referenced in foreign key constraint does not exist

This example has been simplified for clarity, but the original case involved a
bunch of "id" columns everywhere. What's confusing is that "q" is not
actually referenced by the foreign key constraint, but referenced in the
statement that attempts to define the foreign key constraint, so I was looking
on the wrong side of the constraint there.

Attached is a small patch that separates those error messages into:

ERROR: column "q" specified as a constrained column in foreign key constraint
does not exist

ERROR: column "m" specified as a referenced column in foreign key constraint
does not exist

Details may be debatable. Comments?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> I recently had a puzzler, which involved this sort of accidental parser error:
> CREATE TABLE foo (a int, b text, PRIMARY KEY (a, b));

> CREATE TABLE bar (x int, y text, FOREIGN KEY (q, r) REFERENCES foo (m, n));
> ERROR: column "q" referenced in foreign key constraint does not exist

> versus

> CREATE TABLE bar (x int, y text, FOREIGN KEY (x, y) REFERENCES foo (m, n));
> ERROR: column "m" referenced in foreign key constraint does not exist

> This example has been simplified for clarity, but the original case involved a
> bunch of "id" columns everywhere. What's confusing is that "q" is not
> actually referenced by the foreign key constraint, but referenced in the
> statement that attempts to define the foreign key constraint, so I was looking
> on the wrong side of the constraint there.

> Attached is a small patch that separates those error messages into:

It seems to me that the right fix here is not so much to tweak the
message wording as to put in an error location cursor. In more
complicated cases (eg, multiple FOREIGN KEY clauses) the suggested
wording change wouldn't help much anyway.

regards, tom lane

On Monday 06 July 2009 23:00:18 Tom Lane wrote:
> It seems to me that the right fix here is not so much to tweak the
> message wording as to put in an error location cursor. In more
> complicated cases (eg, multiple FOREIGN KEY clauses) the suggested
> wording change wouldn't help much anyway.

It looks like this would involve equipping the Value node with location
information and passing that around everywhere. This could also be used to
supply better location information for a number of other cases. Does that
sound like the right direction?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On Monday 06 July 2009 23:00:18 Tom Lane wrote:
>> It seems to me that the right fix here is not so much to tweak the
>> message wording as to put in an error location cursor. In more
>> complicated cases (eg, multiple FOREIGN KEY clauses) the suggested
>> wording change wouldn't help much anyway.

> It looks like this would involve equipping the Value node with location
> information and passing that around everywhere. This could also be used to
> supply better location information for a number of other cases. Does that
> sound like the right direction?

Yeah, something more or less like that. The trick is to not clutter the
code too much. Perhaps the parser should use an alternate version of
makeString that accepts a location parameter, while leaving existing
calls elsewhere as-is?

regards, tom lane