Обсуждение: Better error message for a small problem with WITH RECURSIVE

Quick, what's wrong with this query?

regression=# with q(x) as (select 1 union all select x+1 from q where x<10)
regression-# select * from q;
ERROR:  relation "q" does not exist
LINE 1: with q(x) as (select 1 union all select x+1 from q where x<1...
      ^
 

The problem is that I forgot to say RECURSIVE.  But the error message
is certainly pretty unhelpful.  The code is following the SQL spec rule,
which says that for a non-recursive WITH query, the query's name isn't
in scope until after you've parsed it.  So you get "does not exist"
rather than something that would clue you in.

I've made this same mistake at least once a day for the past week,
and taken an unreasonable amount of time to figure it out each time :-(
So I think we need a better error message here.

We can't just monkey with the scope rules, because that could change
the meaning of queries that *are* valid, eg a query name could be a
reference to some outer-level WITH.  (Of course you'd have to be pretty
nuts to use the same query name at multiple levels of a single SELECT,
and even more nuts to arrange things so that the intended reference is
not the most closely nested one, but spec is spec.)  What we can do is
keep a list of "not yet parsed WITH-names" in ParseState, and check
through that list when about to fail for relation-not-found, and issue
a suitable message hinting that maybe you forgot RECURSIVE if we find
a match.

I would think this is overkill, except I've made the same darn mistake
one time too many.  It seems clear to me that a lot of other people will
make it too, and if the error message isn't more helpful a lot of time
will get wasted.  Barring loud objections, I'm gonna go change it.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> What we can do is keep a list of "not yet parsed WITH-names" in ParseState,
> and check through that list when about to fail for relation-not-found, and
> issue a suitable message hinting that maybe you forgot RECURSIVE if we find
> a match.
>
> I would think this is overkill, except I've made the same darn mistake
> one time too many.  It seems clear to me that a lot of other people will
> make it too, and if the error message isn't more helpful a lot of time
> will get wasted.  Barring loud objections, I'm gonna go change it.

Perhaps it would be sufficient to just check if we're inside a non-recursive
WITH without bothering to check if the name matches?

--  Gregory Stark EnterpriseDB          http://www.enterprisedb.com Ask me about EnterpriseDB's Slony Replication
support!

Tom Lane wrote:
> I would think this is overkill, except I've made the same darn mistake
> one time too many.  It seems clear to me that a lot of other people will
> make it too, and if the error message isn't more helpful a lot of time
> will get wasted.  Barring loud objections, I'm gonna go change it.

Yes, please. At least DB2 allows recursive queries without the 
"RECURSIVE" keyword, just "WITH" is enough. Without a hint, anyone 
migrating from such a system will spend hours looking at the query, 
seeing nothing wrong.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> What we can do is keep a list of "not yet parsed WITH-names" in ParseState,
>> and check through that list when about to fail for relation-not-found, and
>> issue a suitable message hinting that maybe you forgot RECURSIVE if we find
>> a match.

> Perhaps it would be sufficient to just check if we're inside a non-recursive
> WITH without bothering to check if the name matches?

Even knowing that would require most of the same changes I made to do
the full nine yards, I think.  The previous ParseState info didn't
record anything at all that would allow parserOpenTable to know that
a non-recursive WITH is being examined.
        regards, tom lane

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Yes, please. At least DB2 allows recursive queries without the 
> "RECURSIVE" keyword, just "WITH" is enough. Without a hint, anyone 
> migrating from such a system will spend hours looking at the query, 
> seeing nothing wrong.

Huh, interesting ... so they're violating the letter of the spec
as to WITH name scope.

Anyway, here's what we do as of last night:

regression=# with q(x) as (select 1 union all select x+1 from q where x<10)
select * from q;
ERROR:  relation "q" does not exist
LINE 1: with q(x) as (select 1 union all select x+1 from q where x<1...
      ^
 
DETAIL:  There is a WITH item named "q", but it cannot be referenced from this part of the query.
HINT:  Use WITH RECURSIVE, or re-order the WITH items to remove forward references.
        regards, tom lane