Thread: Fix for tablename in targetlist
We have on the TODO list:
* SELECT pg_class FROM pg_class generates strange error
It passes the tablename as targetlist all the way to the executor, where
it throws an error about Node 704 unkown.
This patch fixes the problem by generating an error in the parser:
test=> select pg_class from pg_class;
ERROR: You can't use a relation alone in a target list.
It passes regression tests.
FYI, I am working down the TODO list, doing items I can handle.
I will apply later if no one objects.
--
Bruce Momjian | http://candle.pha.pa.us
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, Pennsylvania 19026
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.66
diff -c -r1.66 parse_target.c
*** src/backend/parser/parse_target.c 2001/03/22 03:59:41 1.66
--- src/backend/parser/parse_target.c 2001/05/19 03:07:41
***************
*** 55,60 ****
--- 55,63 ----
if (expr == NULL)
expr = transformExpr(pstate, node, EXPR_COLUMN_FIRST);
+ if (IsA(expr, Ident) && ((Ident *) expr)->isRel)
+ elog(ERROR,"You can't use a relation alone in a target list.");
+
type_id = exprType(expr);
type_mod = exprTypmod(expr);
Bruce Momjian writes: > This patch fixes the problem by generating an error in the parser: > > test=> select pg_class from pg_class; > ERROR: You can't use a relation alone in a target list. Maybe it's the parser that's getting it wrong. What if pg_class has a column called pg_class? -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
> Bruce Momjian writes: > > > This patch fixes the problem by generating an error in the parser: > > > > test=> select pg_class from pg_class; > > ERROR: You can't use a relation alone in a target list. > > Maybe it's the parser that's getting it wrong. What if pg_class has a > column called pg_class? The parser doesn't know about columns or table names. It just passes them along. The code checks the indent and sets isRel if it matches something in the range table. Seems like it checks for column matches in the range table first. Looks OK:test=> create table test (test int);CREATEtest=> insert into test values (1);INSERT 1455701test=> select test from test; test ------ 1(1 row) -- Bruce Momjian | http://candle.pha.pa.us 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
Peter Eisentraut <peter_e@gmx.net> writes:
> Bruce Momjian writes:
>> This patch fixes the problem by generating an error in the parser:
>>
>> test=> select pg_class from pg_class;
>> ERROR: You can't use a relation alone in a target list.
> Maybe it's the parser that's getting it wrong. What if pg_class has a
> column called pg_class?
Not an issue: the ambiguous name will be resolved as a column name, and
it will be so resolved before this code executes. We do know at this
point that we have an unadorned relation name; the question is what
to do with it.
I had a thought this morning that raising an error may be the wrong
thing to do. We could instead choose to expand the name into
"pg_class.*", which would take only a little more code and would
arguably do something useful instead of useless. (I suspect that the
fjoin stuff that still remains in the backend was originally designed
to support exactly this interpretation.)
Of course, if we did that, then "select huge_table;" might spit back
a lot of stuff at you before you remembered you'd left off the rest
of the query ;-)
regards, tom lane
> > Maybe it's the parser that's getting it wrong. What if pg_class has a > > column called pg_class? > > Not an issue: the ambiguous name will be resolved as a column name, and > it will be so resolved before this code executes. We do know at this > point that we have an unadorned relation name; the question is what > to do with it. > > I had a thought this morning that raising an error may be the wrong > thing to do. We could instead choose to expand the name into > "pg_class.*", which would take only a little more code and would > arguably do something useful instead of useless. (I suspect that the > fjoin stuff that still remains in the backend was originally designed > to support exactly this interpretation.) > > Of course, if we did that, then "select huge_table;" might spit back > a lot of stuff at you before you remembered you'd left off the rest > of the query ;-) I thought about adding the *. We already allow 'SELECT tab.*'. Should 'SELECT tab' behave the same? It certainly could. Actually, I just tried:test=> select test;ERROR: Attribute 'test' not foundtest=> select test.*; test ------ 1(1 row) Seems a tablename with no FROM clause doesn't get marked as isRel because it is not in the range table to be matched. What would happen if we added auto-star is that a table name in a target list would automatically become tablename.*. Seems it is too prone to cause bad queries to be accepted. -- Bruce Momjian | http://candle.pha.pa.us 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:
> Seems a tablename with no FROM clause doesn't get marked as isRel
> because it is not in the range table to be matched.
> What would happen if we added auto-star is that a table name in a target
> list would automatically become tablename.*. Seems it is too prone to
> cause bad queries to be accepted.
No; the auto-star would only happen if the thing is marked isRel.
So it would just cover the case of "select tab from tab". It seems
reasonable to me --- what other possible interpretation of the meaning
is there?
I tend to agree that we should not change the code to make "select tab"
work, on the grounds of error-proneness.
regards, tom lane
> No; the auto-star would only happen if the thing is marked isRel.
> So it would just cover the case of "select tab from tab". It seems
> reasonable to me --- what other possible interpretation of the meaning
> is there?
>
> I tend to agree that we should not change the code to make "select tab"
> work, on the grounds of error-proneness.
OK, here is another patch that does this:
test=> select test from test;
test
------
1
(1 row)
It is small to I am attaching it here.
--
Bruce Momjian | http://candle.pha.pa.us
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, Pennsylvania 19026
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.66
diff -c -r1.66 parse_target.c
*** src/backend/parser/parse_target.c 2001/03/22 03:59:41 1.66
--- src/backend/parser/parse_target.c 2001/05/19 17:18:11
***************
*** 154,166 ****
}
else
{
! /* Everything else but Attr */
! p_target = lappend(p_target,
! transformTargetEntry(pstate,
! res->val,
! NULL,
! res->name,
! false));
}
targetlist = lnext(targetlist);
--- 154,183 ----
}
else
{
! Node *rteorjoin;
! int sublevels_up;
!
! if (IsA(res->val, Ident) &&
! (rteorjoin = refnameRangeOrJoinEntry(pstate,
! ((Ident *) res->val)->name,
! &sublevels_up)) != NULL &&
! IsA(rteorjoin, RangeTblEntry))
! {
! /* Expand SELECT tab FROM tab; to SELECT tab.* FROM tab; */
! p_target = nconc(p_target,
! expandRelAttrs(pstate,
! (RangeTblEntry *) rteorjoin));
! }
! else
! {
! /* Everything else */
! p_target = lappend(p_target,
! transformTargetEntry(pstate,
! res->val,
! NULL,
! res->name,
! false));
! }
}
targetlist = lnext(targetlist);
Tom Lane writes: > I had a thought this morning that raising an error may be the wrong > thing to do. We could instead choose to expand the name into > "pg_class.*", Yeah, and maybe if there's a database called pg_class we select from all tables at once. In fact, this should be the default behaviour if I just type 'SELECT;'. No really, I don't see a point of not enforcing the correct syntax, when adding '.*' is all it takes to get the alternative behaviour in a standard way. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes:
> No really, I don't see a point of not enforcing the correct syntax, when
> adding '.*' is all it takes to get the alternative behaviour in a standard
> way.
True, although there's a certain inconsistency in allowing a whole row
to be passed to a function by
select foo(pg_class) from pg_class;
and not allowing the same row to be output by
select pg_class from pg_class;
I don't feel strongly about it though.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, here is another patch that does this:
That seems considerably uglier than your first patch. In particular,
why aren't you looking for isRel being set in the Ident node? It
looks to me like you may have changed the behavior in the case where
the Ident could be either a table or column name.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > OK, here is another patch that does this: > > That seems considerably uglier than your first patch. In particular, > why aren't you looking for isRel being set in the Ident node? It > looks to me like you may have changed the behavior in the case where > the Ident could be either a table or column name. OK, here is a new patch. I thought I had to go through transformTargetEntry() -> transformExpr() -> transformIdent() to get Ident.isRel set. Seems it is set earlier too, so the new code is shorter. I am still researching the purpose of Ident.isRel. If someone knows, please chime in. I know it says the Ident is a relation, but why have a field when you can look it up in the rangetable? -- Bruce Momjian | http://candle.pha.pa.us 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, Pennsylvania 19026 Index: doc/TODO =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/doc/TODO,v retrieving revision 1.464 diff -c -r1.464 TODO *** doc/TODO 2001/05/18 16:28:12 1.464 --- doc/TODO 2001/05/20 00:35:29 *************** *** 140,146 **** * Allow RULE recompilation * Add BETWEEN ASYMMETRIC/SYMMETRIC * Change LIMIT val,val to offset,limit to match MySQL ! * Allow Pl/PgSQL's RAISE function to take expressions * ALTER * ALTER TABLE ADD COLUMN to inherited table put column in wrong place [inheritance] --- 140,146 ---- * Allow RULE recompilation * Add BETWEEN ASYMMETRIC/SYMMETRIC * Change LIMIT val,val to offset,limit to match MySQL ! * Allow PL/PgSQL's RAISE function to take expressions * ALTER * ALTER TABLE ADD COLUMN to inherited table put column in wrong place [inheritance] Index: src/backend/parser/parse_target.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/parser/parse_target.c,v retrieving revision 1.66 diff -c -r1.66 parse_target.c *** src/backend/parser/parse_target.c 2001/03/22 03:59:41 1.66 --- src/backend/parser/parse_target.c 2001/05/20 00:35:30 *************** *** 154,166 **** } else { ! /* Everything else but Attr */ ! p_target = lappend(p_target, ! transformTargetEntry(pstate, ! res->val, ! NULL, ! res->name, ! false)); } targetlist = lnext(targetlist); --- 154,182 ---- } else { ! Node *rteorjoin; ! int sublevels_up; ! ! if (IsA(res->val, Ident) && ((Ident *) res->val)->isRel) ! { ! rteorjoin = refnameRangeOrJoinEntry(pstate, ! ((Ident *) res->val)->name, ! &sublevels_up); ! Assert(rteorjoin != NULL); ! p_target = nconc(p_target, ! expandRelAttrs(pstate, ! (RangeTblEntry *) rteorjoin)); ! } ! else ! { ! /* Everything else */ ! p_target = lappend(p_target, ! transformTargetEntry(pstate, ! res->val, ! NULL, ! res->name, ! false)); ! } } targetlist = lnext(targetlist);
> > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > OK, here is another patch that does this: > > > > That seems considerably uglier than your first patch. In particular, > > why aren't you looking for isRel being set in the Ident node? It > > looks to me like you may have changed the behavior in the case where > > the Ident could be either a table or column name. > > OK, here is a new patch. I thought I had to go through > transformTargetEntry() -> transformExpr() -> transformIdent() to get > Ident.isRel set. Seems it is set earlier too, so the new code is > shorter. I am still researching the purpose of Ident.isRel. If someone > knows, please chime in. I know it says the Ident is a relation, but why > have a field when you can look it up in the rangetable? This patch was no good. It worked only because I had created test as: CREATE TABLE test ( test int); to test Peter's test of matching column/table names. In fact, I was right that you have to call transformTargetEntry() -> transformExpr() -> transformIdent() to get isRel set, and I have to do the longer fix. -- Bruce Momjian | http://candle.pha.pa.us 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:
> In fact, I was
> right that you have to call transformTargetEntry() -> transformExpr() ->
> transformIdent() to get isRel set, and I have to do the longer fix.
Yes, I would think that you should do transformTargetEntry() first and
then look to see if you have an Ident w/ isRel set. The initial patch
was OK because it happened after that transformation.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > In fact, I was > > right that you have to call transformTargetEntry() -> transformExpr() -> > > transformIdent() to get isRel set, and I have to do the longer fix. > > Yes, I would think that you should do transformTargetEntry() first and > then look to see if you have an Ident w/ isRel set. The initial patch > was OK because it happened after that transformation. Yes, and you can't do that later because you need to add to the target list. Calling transformTargetEntry returns a Resdom, which I don't think I can tell if that is a rel or not, and it mucks up pstate->p_last_resno++. I will show the patch on the patches list. It does things similar to what happens above in that function. -- Bruce Momjian | http://candle.pha.pa.us 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
On Sat, May 19, 2001 at 10:50:31AM -0400, Tom Lane wrote: > I had a thought this morning that raising an error may be the wrong > thing to do. We could instead choose to expand the name into > "pg_class.*", which would take only a little more code and would > arguably do something useful instead of useless. (I suspect that the > fjoin stuff that still remains in the backend was originally designed > to support exactly this interpretation.) This is almost certainly the wrong thing to do. It would introduce ambiguity to the syntax, that can only be error prone in the long run. What happens if people put that kind of query in a view, or hard coded into a program somewhere, then later decide to ALTER TABLE to add a column by that name? If somebody forgets the ".*", they should reasonably expect an error message. (And, I would personally be annoyed if I didn't get one, and instead my incorrect query went through) -- Michael Samuel <michael@miknet.net>
Bruce,
On Fri, 18 May 2001, Bruce Momjian wrote:
> We have on the TODO list:
>
> * SELECT pg_class FROM pg_class generates strange error
>
> It passes the tablename as targetlist all the way to the executor, where
> it throws an error about Node 704 unkown.
The problem is caused in transformIdent() (parse_expr.c):
if (ident->indirection == NIL && refnameRangeTableEntry(pstate, ident->name) != NULL) {
ident->isRel = TRUE; result = (Node *) ident; }
It is pretty clear what is happening here. ident->name is a member of
range table so the type of ident is not changed, as would be the case with
an attribute. Commenting this code out means that result = NULL and the
error 'Attribute 'pg_class' not found'. This, in my opinion, is the
correct error to be generated. Moreover, I cannot find any flow on effect
which may result from removing this code -- regression tests all
pass. From what I can tell, all transformations of Nodes which are of type
Ident should have already been transformed anyway -- have I over looked
something?
Gavin
> Bruce,
>
> On Fri, 18 May 2001, Bruce Momjian wrote:
>
> > We have on the TODO list:
> >
> > * SELECT pg_class FROM pg_class generates strange error
> >
> > It passes the tablename as targetlist all the way to the executor, where
> > it throws an error about Node 704 unkown.
>
> The problem is caused in transformIdent() (parse_expr.c):
>
> if (ident->indirection == NIL &&
> refnameRangeTableEntry(pstate, ident->name) != NULL)
> {
> ident->isRel = TRUE;
> result = (Node *) ident;
> }
>
> It is pretty clear what is happening here. ident->name is a member of
> range table so the type of ident is not changed, as would be the case with
> an attribute. Commenting this code out means that result = NULL and the
> error 'Attribute 'pg_class' not found'. This, in my opinion, is the
> correct error to be generated. Moreover, I cannot find any flow on effect
> which may result from removing this code -- regression tests all
> pass. From what I can tell, all transformations of Nodes which are of type
> Ident should have already been transformed anyway -- have I over looked
> something?
I am confused. I thought I fixed this about a month ago. Do we need
more coded added here?
You are suggesting throwing an error as soon as an idend appears as a
relation. I don't know enough about the code to be sure that is OK. I
realize the regression tests pass.
-- Bruce Momjian | http://candle.pha.pa.us 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,
On Tue, 12 Jun 2001, Bruce Momjian wrote:
> > Bruce,
> >
> > On Fri, 18 May 2001, Bruce Momjian wrote:
> >
> > > We have on the TODO list:
> > >
> > > * SELECT pg_class FROM pg_class generates strange error
> > >
> > > It passes the tablename as targetlist all the way to the executor, where
> > > it throws an error about Node 704 unkown.
> >
> > The problem is caused in transformIdent() (parse_expr.c):
> >
> > if (ident->indirection == NIL &&
> > refnameRangeTableEntry(pstate, ident->name) != NULL)
> > {
> > ident->isRel = TRUE;
> > result = (Node *) ident;
> > }
> >
> > It is pretty clear what is happening here. ident->name is a member of
> > range table so the type of ident is not changed, as would be the case with
> > an attribute. Commenting this code out means that result = NULL and the
> > error 'Attribute 'pg_class' not found'. This, in my opinion, is the
> > correct error to be generated. Moreover, I cannot find any flow on effect
> > which may result from removing this code -- regression tests all
> > pass. From what I can tell, all transformations of Nodes which are of type
> > Ident should have already been transformed anyway -- have I over looked
> > something?
>
> I am confused. I thought I fixed this about a month ago. Do we need
> more coded added here?
>
> You are suggesting throwing an error as soon as an idend appears as a
> relation. I don't know enough about the code to be sure that is OK. I
> realize the regression tests pass.
Removing the said code and not applying your patch allows the parser to
recognise that pg_class is not an attribute of pg_class relation. There
does not seem to be any side effect from removing this code, though I
would like to see if someone can find fault in that. If there is no
problem, then -- in light of the discussion on this a month or so ago --
SELECT pg_class FROM pg_class should be be considered 'select the column
pg_class from the pg_class relation' which is the same as SELECT
nosuchcolumn FROM pg_class. Isn't this the most effective way to solve the
problem then?
Thanks
Gavin
> > You are suggesting throwing an error as soon as an idend appears as a > > relation. I don't know enough about the code to be sure that is OK. I > > realize the regression tests pass. > > Removing the said code and not applying your patch allows the parser to > recognise that pg_class is not an attribute of pg_class relation. There > does not seem to be any side effect from removing this code, though I > would like to see if someone can find fault in that. If there is no > problem, then -- in light of the discussion on this a month or so ago -- > SELECT pg_class FROM pg_class should be be considered 'select the column > pg_class from the pg_class relation' which is the same as SELECT > nosuchcolumn FROM pg_class. Isn't this the most effective way to solve the > problem then? Uh..., I don't know. -- Bruce Momjian | http://candle.pha.pa.us 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
> On Tue, 12 Jun 2001, Bruce Momjian wrote:
>> I am confused. I thought I fixed this about a month ago. Do we need
>> more coded added here?
You did, and we don't. In current sources:
regression=# SELECT pg_class FROM pg_class;
ERROR: You can't use relation names alone in the target list, try relation.*.
regression=#
One might quibble with the wording of the error message, but at least
it's to the point.
regards, tom lane