Thread: New warning code for missing FROM relations

New warning code for missing FROM relations

From
Bruce Momjian
Date:
I have committed new warning code to alert users who auto-create
relations without knowing it.

The new code does not throw a warning for:
SELECT pg_language.*;

but does throw one for:
SELECT pg_language.* FROM pg_class

The code issues the warning if it auto-creates a range table entry, and
there is already a range table entry identified as coming from a FROM
clause.  Correlated subqueries should not be a problem because they are
not auto-created.

The regression tests run fine, except for:SELECT *   INTO TABLE tmp1   FROM tmp   WHERE onek.unique1 < 2;NOTICE:
Addingmissing FROM-clause entry for table onekDROP TABLE tmp1;SELECT *   INTO TABLE tmp1   FROM tmp   WHERE
onek2.unique1< 2;NOTICE:  Adding missing FROM-clause entry for table onek2
 

Seems those warnings are justified.  In fact, I am not even sure what
these queries are trying to do.  I have modified the expected files so
they now expect to see the warnings.

A bigger question is whether we should issue ERROR for these queries. 
If they have already used a FROM clause, why would they have other
relations not specified there?

If people have other suggestions about this, I would be glad to modify
the code.  A new function warnAutoRange() does the checking.

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I have committed new warning code to alert users who auto-create
> relations without knowing it.
> The code issues the warning if it auto-creates a range table entry, and
> there is already a range table entry identified as coming from a FROM
> clause.  Correlated subqueries should not be a problem because they are
> not auto-created.

I still prefer the suggestion I made before: complain only if the
implicit FROM entry is for a table already present in the rangelist
(under a different alias, obviously).  The fact that that choice
would not break any existing regression tests seems relevant...
        regards, tom lane


Re: New warning code for missing FROM relations

From
Bruce Momjian
Date:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I have committed new warning code to alert users who auto-create
> > relations without knowing it.
> > The code issues the warning if it auto-creates a range table entry, and
> > there is already a range table entry identified as coming from a FROM
> > clause.  Correlated subqueries should not be a problem because they are
> > not auto-created.
> 
> I still prefer the suggestion I made before: complain only if the
> implicit FROM entry is for a table already present in the rangelist
> (under a different alias, obviously).  The fact that that choice
> would not break any existing regression tests seems relevant...

But it seems mine is going to complain if they forget one in a FROM
clause, which sort of makes sense to me.   I can do your suggestion, but
this makes more sense.  Can we get some other votes?

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> I still prefer the suggestion I made before: complain only if the
>> implicit FROM entry is for a table already present in the rangelist
>> (under a different alias, obviously).  The fact that that choice
>> would not break any existing regression tests seems relevant...

> But it seems mine is going to complain if they forget one in a FROM
> clause, which sort of makes sense to me.

Seems like the real question is what is the goal of having the warning.
Are we (a) trying to nag people into writing their queries in an
SQL-compliant way, or are we (b) trying to warn about probable mistakes
while still considering implicit FROM entries as a fully supported
Postgres feature?

If the goal is (a) then your way is better, but I like mine if the goal
is (b).  Seems like some discussion is needed here about just what we
want to accomplish.
        regards, tom lane


Re: New warning code for missing FROM relations

From
Bruce Momjian
Date:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> I still prefer the suggestion I made before: complain only if the
> >> implicit FROM entry is for a table already present in the rangelist
> >> (under a different alias, obviously).  The fact that that choice
> >> would not break any existing regression tests seems relevant...
> 
> > But it seems mine is going to complain if they forget one in a FROM
> > clause, which sort of makes sense to me.
> 
> Seems like the real question is what is the goal of having the warning.
> Are we (a) trying to nag people into writing their queries in an
> SQL-compliant way, or are we (b) trying to warn about probable mistakes
> while still considering implicit FROM entries as a fully supported
> Postgres feature?
> 
> If the goal is (a) then your way is better, but I like mine if the goal
> is (b).  Seems like some discussion is needed here about just what we
> want to accomplish.

I agree the goal is (b).  However, I can not imagine a query with a FROM
clause that would ever want to use auto-creation of range entries.

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Philip Warner
Date:
At 00:40 3/06/00 -0400, Bruce Momjian wrote:
>
>The regression tests run fine, except for:
>    
>    SELECT *
>       INTO TABLE tmp1
>       FROM tmp
>       WHERE onek.unique1 < 2;
>    NOTICE:  Adding missing FROM-clause entry for table onek

Personally I would prefer this to generate an error, eg:
   Table 'onek' refereenced in the WHERE clause is not in the FROM clause.

Is is worth adding yet another setting, eg. set sql92=strict, which would
disallow such flagrant breaches of the standard? Maybe it could even be set
as the default in template1? I understand that breaking legacy code is a
bad idea, so the warning is a good step, but I would prefer an error if I
ever write such a statement.

Other DBs I've worked with issue warnings for several version before
changing default behaviour, so perhaps in version 8.0, the above code could
prduce an error by default (unless 'set sql92=relaxed' was specified).

P.S. Given that 'set sql92=xxx' may imply that we are enforcing compliance,
which is unlikely, maybe it should be 'set implied-tables' or something
more specific and meaningful. I have no idea how the 'set' command works,
but to avoid a whole lot of new variables, maybe 'set
sql-compiance-options=no-implied-tables, no-something-else,
allow-another-thing' would allow for expansion...



----------------------------------------------------------------
Philip Warner                    |     __---_____
Albatross Consulting Pty. Ltd.   |----/       -  \
(A.C.N. 008 659 498)             |          /(@)   ______---_
Tel: +61-03-5367 7422            |                 _________  \
Fax: +61-03-5367 7430            |                 ___________ |
Http://www.rhyme.com.au          |                /           \|                                |    --________--
PGP key available upon request,  |  /
and from pgp5.ai.mit.edu:11371   |/


Re: New warning code for missing FROM relations

From
Bruce Momjian
Date:
> At 00:40 3/06/00 -0400, Bruce Momjian wrote:
> >
> >The regression tests run fine, except for:
> >    
> >    SELECT *
> >       INTO TABLE tmp1
> >       FROM tmp
> >       WHERE onek.unique1 < 2;
> >    NOTICE:  Adding missing FROM-clause entry for table onek
> 
> Personally I would prefer this to generate an error, eg:
> 
>     Table 'onek' refereenced in the WHERE clause is not in the FROM clause.

Yes, that was one of my stated options, return an error.

> Is is worth adding yet another setting, eg. set sql92=strict, which would
> disallow such flagrant breaches of the standard? Maybe it could even be set
> as the default in template1? I understand that breaking legacy code is a
> bad idea, so the warning is a good step, but I would prefer an error if I
> ever write such a statement.

I am concerned about overloading the SET command.  Seems we should just
agree on a behavior.

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Philip Warner
Date:
>
>> Is is worth adding yet another setting, eg. set sql92=strict, which would
>> disallow such flagrant breaches of the standard? Maybe it could even be set
>> as the default in template1? I understand that breaking legacy code is a
>> bad idea, so the warning is a good step, but I would prefer an error if I
>> ever write such a statement.
>
>I am concerned about overloading the SET command.  Seems we should just
>agree on a behavior.
>

Then make it an option at build time.



----------------------------------------------------------------
Philip Warner                    |     __---_____
Albatross Consulting Pty. Ltd.   |----/       -  \
(A.C.N. 008 659 498)             |          /(@)   ______---_
Tel: +61-03-5367 7422            |                 _________  \
Fax: +61-03-5367 7430            |                 ___________ |
Http://www.rhyme.com.au          |                /           \|                                |    --________--
PGP key available upon request,  |  /
and from pgp5.ai.mit.edu:11371   |/


Re: New warning code for missing FROM relations

From
"Zeugswetter Andreas"
Date:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > I have committed new warning code to alert users who auto-create
> > > relations without knowing it.
> > > The code issues the warning if it auto-creates a range table entry, and
> > > there is already a range table entry identified as coming from a FROM
> > > clause.  Correlated subqueries should not be a problem because they are
> > > not auto-created.
> > 
> > I still prefer the suggestion I made before: complain only if the
> > implicit FROM entry is for a table already present in the rangelist
> > (under a different alias, obviously).  The fact that that choice
> > would not break any existing regression tests seems relevant...
> 
> But it seems mine is going to complain if they forget one in a FROM
> clause, which sort of makes sense to me.   I can do your suggestion, but
> this makes more sense.  Can we get some other votes?

I like it the way you did it. Personally I would even throw an error,
but that would probably be too strict. 

I would change the regressiontest to add onek to the from clause, 
and not make it throw the warning. 
Imho this example is only good to demonstrate how you can 
misuse a feature.

There are good examples for using it, but all of those that I can think of 
don't have a from clause.

Andreas



Re: New warning code for missing FROM relations

From
"Zeugswetter Andreas"
Date:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > >> I still prefer the suggestion I made before: complain only if the
> > >> implicit FROM entry is for a table already present in the rangelist
> > >> (under a different alias, obviously).  The fact that that choice
> > >> would not break any existing regression tests seems relevant...
> > 
> > > But it seems mine is going to complain if they forget one in a FROM
> > > clause, which sort of makes sense to me.
> > 
> > Seems like the real question is what is the goal of having the warning.
> > Are we (a) trying to nag people into writing their queries in an
> > SQL-compliant way, or are we (b) trying to warn about probable mistakes
> > while still considering implicit FROM entries as a fully supported
> > Postgres feature?
> > 
> > If the goal is (a) then your way is better, but I like mine if the goal
> > is (b).  Seems like some discussion is needed here about just what we
> > want to accomplish.
> 
> I agree the goal is (b).  However, I can not imagine a query with a FROM
> clause that would ever want to use auto-creation of range entries.

how about:

delete from taba where a=tabb.a; 

I think the implicit auto-creation should only be disallowed/warned in 
select statements that have a from clause, not update and delete.

Andreas



Re: New warning code for missing FROM relations

From
Bruce Momjian
Date:
> > > If the goal is (a) then your way is better, but I like mine if the goal
> > > is (b).  Seems like some discussion is needed here about just what we
> > > want to accomplish.
> > 
> > I agree the goal is (b).  However, I can not imagine a query with a FROM
> > clause that would ever want to use auto-creation of range entries.
> 
> how about:
> 
> delete from taba where a=tabb.a; 
> 
> I think the implicit auto-creation should only be disallowed/warned in 
> select statements that have a from clause, not update and delete.

I meant a SELECT FROM clause.  A DELETE FROM is not the same, and does
not mark the entry as inFromCl.  I should have been more specific.

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Bruce Momjian
Date:
> I like it the way you did it. Personally I would even throw an error,
> but that would probably be too strict. 

Yes, I figured the same.

> 
> I would change the regressiontest to add onek to the from clause, 
> and not make it throw the warning. 
> Imho this example is only good to demonstrate how you can 
> misuse a feature.

OK.

> 
> There are good examples for using it, but all of those that I can think of 
> don't have a from clause.

Yes, that was my logic.

--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 


Re: New warning code for missing FROM relations

From
Peter Eisentraut
Date:
Philip Warner writes:

> >    SELECT *
> >       INTO TABLE tmp1
> >       FROM tmp
> >       WHERE onek.unique1 < 2;
> >    NOTICE:  Adding missing FROM-clause entry for table onek

> Is is worth adding yet another setting, eg. set sql92=strict, which
> would disallow such flagrant breaches of the standard?

SQL provides for facility called the SQL Flagger, which is supposed to do
exactly that. This might sound like an interesting idea but in order for
it to be useful you'd have to maintain it across the board, which sounds
like a major head ache.

The irony in the given example is that the SELECT INTO command isn't in
the standard in the first place so you'd have to create all sorts of
double standards. Certain things would be "extensions", certain things
would be "misuse". And for all it's worth, we have no idea which is which.

If you want to throw about warnings about "probable" coding errors and the
like one *must* be able to switch them off. Either something is right,
then you shut up. Or it's wrong, then you throw an error. Or you're not
sure, then you better leave it up to the user.


-- 
Peter Eisentraut                  Sernanders väg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden