Re: Improving "missing FROM-clause entry" message

I'm thinking about whether we can't improve the message for "missing
FROM-clause entry" to somehow account for situations where the table
does exist in the query but it's referenced from an improper place,
as in bug #2130 (filed a couple hours ago, not yet visible in mail list
archives):

SELECT ... FROM a, b LEFT JOIN c ON (c.task_id=a.task_id ...

This seems to come up often enough in porting MySQL code that we ought
to try to deliver a more specific error message for it.

We can fairly easily modify warnAutoRange() to check whether the target
name exists anywhere in the ParseState's range table. This would not
detect forward references, as in

SELECT ... FROM b LEFT JOIN c ON (c.task_id=a.task_id ...), a ...

but I think this is acceptable since that case isn't going to occur
in ported MySQL code.

What I'm wondering about is how to word the error message. A
minimum-change approach would be to add a HINT, but I'm thinking
it'd be better to replace the message entirely:

ERROR: reference to table "a" is not allowed from this location
HINT: JOIN clauses cannot refer to tables outside the JOIN

Any thoughts about it? Are there any cases where the existing message
wording is preferable even though a matching RTE does exist?

regards, tom lane

I wrote:
> I'm thinking about whether we can't improve the message for "missing
> FROM-clause entry" to somehow account for situations where the table
> does exist in the query but it's referenced from an improper place,
> as in bug #2130 (filed a couple hours ago, not yet visible in mail list
> archives):
> SELECT ... FROM a, b LEFT JOIN c ON (c.task_id=a.task_id ...

On further investigation, this is arguably a regression in 8.1.
Every PG release back to 7.2 has responded to this query with

NOTICE: adding missing FROM-clause entry for table "a"
ERROR: JOIN/ON clause refers to "a", which is not part of JOIN

In 8.1, where add_missing_from defaults to false, you get the first
line as an ERROR and so the much-more-useful specific message doesn't
appear. I think we need to do something about this.

regards, tom lane

I wrote:
>> I'm thinking about whether we can't improve the message for "missing
>> FROM-clause entry" to somehow account for situations where the table
>> does exist in the query but it's referenced from an improper place,
>> ...

> On further investigation, this is arguably a regression in 8.1.
> Every PG release back to 7.2 has responded to this query with

> NOTICE: adding missing FROM-clause entry for table "a"
> ERROR: JOIN/ON clause refers to "a", which is not part of JOIN

> In 8.1, where add_missing_from defaults to false, you get the first
> line as an ERROR and so the much-more-useful specific message doesn't
> appear. I think we need to do something about this.

After some thought I've come up with possible ways to handle this.

Plan A: when we are about to raise an error in warnAutoRange(),
scan the rangetable to see if there are any entries anywhere that could
match the specified table name (either by alias or by real table name).
If so, don't use the "missing FROM-clause entry" wording, but instead
say something like

ERROR: invalid reference to FROM-clause entry for table "foo"
HINT: The entry cannot be referenced from this part of the query.

When the match is by real table name and there's an alias, a better HINT
might be

HINT: You probably should have used the table alias "bar".

since this would do something useful for the perennial mistake

select foo.* from foo f;

Plan B: when we are about to raise an error in warnAutoRange(), instead
just save the error info in the ParseState struct and keep going. If
we get to the end of parsing without detecting any other error, report
the missing-FROM error. This would let the specific error message
ERROR: JOIN/ON clause refers to "a", which is not part of JOIN
come out when it's applicable, but not change the behavior otherwise.

Plan C: do both. This would give us the most specific error messages
possible without major restructuring.

A reasonable objection to either Plan A or Plan C is that it will add
error strings that are not currently in the translation message files;
which wouldn't matter for a HEAD-only patch, but I'd really like to
back-patch this into 8.1. Plan B wouldn't change the set of possible
messages.

If that's not considered a show-stopper, I'd like to go with Plan C.
We've certainly got plenty of evidence that this is a confusing error
condition, and the more we can do to explain the problem in the error
message, the less time will be wasted all around.

Comments? Any thoughts about the exact wording of the proposed new
messages?

regards, tom lane

On Wednesday 04 January 2006 20:37, Tom Lane wrote:
> A reasonable objection to either Plan A or Plan C is that it will add
> error strings that are not currently in the translation message files;
> which wouldn't matter for a HEAD-only patch, but I'd really like to
> back-patch this into 8.1. Plan B wouldn't change the set of possible
> messages.
>

No objections here but since I don't use a foreign lang I figure my vote
doesn't really matter. I was wondering though if it would be resonable to try
and get some language updates into the patch release?

--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
> No objections here but since I don't use a foreign lang I figure my vote
> doesn't really matter. I was wondering though if it would be resonable to try
> and get some language updates into the patch release?

With the current re-release plans it'd take awfully quick response from
the translators to get that done for 8.1.2. (I'm not even 100% sure
I'll get the code patch done for 8.1.2, though I'll try if there's
nothing more important on my plate tomorrow.) But it's reasonable
to hope that the translations might catch up in 8.1.3 or later.

regards, tom lane

Tom Lane wrote:
> Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
> > No objections here but since I don't use a foreign lang I figure my vote
> > doesn't really matter. I was wondering though if it would be resonable to try
> > and get some language updates into the patch release?
>
> With the current re-release plans it'd take awfully quick response from
> the translators to get that done for 8.1.2. (I'm not even 100% sure
> I'll get the code patch done for 8.1.2, though I'll try if there's
> nothing more important on my plate tomorrow.) But it's reasonable
> to hope that the translations might catch up in 8.1.3 or later.

Do we have enough time to test the patch before the minor releases?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Do we have enough time to test the patch before the minor releases?

Sure, it's not like it raises any portability issues. As long as it
gives a better error message than before in some common cases, it'll
be a step forward, even if we think of further improvements later.

regards, tom lane