Fix for tablename in targetlist

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);
CREATE
test=> insert into test values (1);
INSERT 145570 1
test=> 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, Pennsylvania 19026

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 found
test=> 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, Pennsylvania 19026

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

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

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, Pennsylvania 19026

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, Pennsylvania 19026

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)

I object also. It is not a feature, and it might stand in the way of a future interpretation.
What does it buy you except saving the 2 chars '.*' ? Imho it is far from obvious behavior.

Andreas

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

Imho there is a big difference between the two. The foo(pg_class) calls a function
with argument type opaque or type pg_class.
I would go so far as to say, that above foo function call would have a
different meaning if written with 'pg_class.*'.

select foo(pg_class.*) from pg_class;

Could be interpreted as calling a function foo with pg_class ncolumns
arguments of the corresponding types.

select pg_class from pg_class;

Probably a valid interpretation would be if type pg_class or opaque had an
output function.

Andreas

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

select pg_class from pg_class;

Probably a valid interpretation would be if type pg_class or opaque had an
output function.

Hmm, good point. We shouldn't foreclose the possibility of handling
things that way. Okay, I'm convinced: allowing .* to be omitted isn't
a good idea.

regards, tom lane

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

select pg_class from pg_class;

Probably a valid interpretation would be if type pg_class or opaque had an
output function.

Hmm, good point. We shouldn't foreclose the possibility of handling
things that way. Okay, I'm convinced: allowing .* to be omitted isn't
a good idea.

OK, old patch causing an elog being added now.

-- 
  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

Patch applied and attached. TODO updated.

Zeugswetter Andreas SB <ZeugswetterA@wien.spardat.at> writes:

select pg_class from pg_class;

Probably a valid interpretation would be if type pg_class or opaque had an
output function.

Hmm, good point. We shouldn't foreclose the possibility of handling
things that way. Okay, I'm convinced: allowing .* to be omitted isn't
a good idea.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://www.postgresql.org/search.mpl

-- 
  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

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, Pennsylvania 19026

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, Pennsylvania 19026

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

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