Thread: New warning code for missing FROM relations
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
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
> 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
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
> 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
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 |/
> 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
> >> 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 |/
> > 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
> > 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
> > > 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
> 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
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