Bug with sequences in 6.4.2

Greetings,
I have found the place from which this bug seems to originate in
[6.4.2 sources]
backend/parser/parse_func.c
line #443 - #445

There is the call to
text *lower(text *)

so probably any fix should start here.
Now what is the intended semantics/syntax for nextval(<ARG>) ?

e.g.
a) nextval('AA'),
b) nextval("AA")
or even
c) nextval('"AA"')

a change to allow syntax a) might have the potential to break existing
assumptions about case-insensitiveness on behalf of nextval,
I would prefer a change that would allow
nextval(aa) -- This will cause problems in the parser as a syntactic
-- element is removed
nextval("AA")
something around this line of thought will make handling of identifiers
consistent again. Treat a string case-insensitive unless it is a quoted
identifier.
I had expected the ugly c) above to work, but currently there is no check if
the sequencename might be a quoted identifier.
I'm not sure if I have enough time to make a patch myself, as I'm not overly
confident to find my way through the intestines of the parser, but maybe one
of you can either think of a quick fix or point me to what needs to be
considered to implement a fix.

BTW, as this affects SERIAL, I consider this a definite showstopper for both
6.4.x and 6.5 ( that is if it is still present in there ).

Test Case from my earlier post should probably go into the Regress tests ASAP.

With Regards,
Stefan Wehner

Greetings,
there seems to be a slight bug in the parsing of the
nextval function, tested under 6.4.2.
It affects also the SERIAL type.

Symptom :

CREATE SEQUENCE "AA";
-- Correct, quoted identifier is allowed;
SELECT NEXTVAL('AA');
--Produces Error
--aa.nextval: sequence does not exist

Let me comment on this. In the first statement, "AA" is used in an
SQL command, and we handle this correctly. In the second case, NEXTVAL
is a function, called with a string.

Now in parse_func.c, we convert 'AA' to lower to try and find the
sequence table. My assumption is that we should attempt to find the
table without doing a lower(), and if that fails, try lower.

Does that make sense to people. We can't just lower it in every case.

Probable source of problem, the Argument to nextval is
not handled correctly as an Table Identifier.

E.g. nextval('"AA"') is generates
"aa".nextval: sequence does not exist

Note the lowercase between the quotes.

I quickly browsed the sources, but have not found the
place where the conversion to lowercase occurs.

Please check this against 6.5 and 6.4.3 too.

With Regards,
Stefan Wehner

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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

<problem with case of sequence name snipped>

Now in parse_func.c, we convert 'AA' to lower to try and find the
sequence table. My assumption is that we should attempt to find the
table without doing a lower(), and if that fails, try lower.

Does that make sense to people. We can't just lower it in every case.

Sounds line exactly the right fix to me. Oh, and a quick scan for other
cases of this assumption (auto lower() of field and attribute names) would
be nice, too.

Ross

--
Ross J. Reedstrom, Ph.D., <reedstrm@rice.edu>
NSBRI Research Scientist/Programmer
Computer and Information Technology Institute
Rice University, 6100 S. Main St., Houston, TX 77005

Greetings,
there seems to be a slight bug in the parsing of the
nextval function, tested under 6.4.2.
It affects also the SERIAL type.

Symptom :

CREATE SEQUENCE "AA";
-- Correct, quoted identifier is allowed;
SELECT NEXTVAL('AA');
--Produces Error
--aa.nextval: sequence does not exist

Probable source of problem, the Argument to nextval is
not handled correctly as an Table Identifier.

E.g. nextval('"AA"') is generates
"aa".nextval: sequence does not exist

Note the lowercase between the quotes.

I quickly browsed the sources, but have not found the
place where the conversion to lowercase occurs.

OK, I have made the following change to allow case-sensitive handling of
nextval. It tries the exact case first, then tries lowercase.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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 <maillist@candle.pha.pa.us> writes:

CREATE SEQUENCE "AA";
-- Correct, quoted identifier is allowed;
SELECT NEXTVAL('AA');
--Produces Error

Let me comment on this. In the first statement, "AA" is used in an
SQL command, and we handle this correctly. In the second case, NEXTVAL
is a function, called with a string.
Now in parse_func.c, we convert 'AA' to lower to try and find the
sequence table. My assumption is that we should attempt to find the
table without doing a lower(), and if that fails, try lower.
Does that make sense to people. We can't just lower it in every case.

That would create an ambiguity that is better avoided. I think nextval
ought to duplicate the parser's behavior --- if possible, actually call
the same routine the parser uses for looking up a sequence name.
I suggest that it operate like this:

(1) nextval('AA') operates on sequence aa

AA is lowercased, same as unquoted AA would be by the parser.

(2) nextval('"AA"') operates on sequence AA

Quoted "AA" is treated as AA, same as parser would do it.

This should be fully backward compatible with existing SQL code, since
the existing nextval() code implements case (1). I doubt anyone has
tried putting double quotes into their nextval arguments, so adding
the case (2) behavior shouldn't break anything.

regards, tom lane

Bruce Momjian <maillist@candle.pha.pa.us> writes:

CREATE SEQUENCE "AA";
-- Correct, quoted identifier is allowed;
SELECT NEXTVAL('AA');
--Produces Error

Let me comment on this. In the first statement, "AA" is used in an
SQL command, and we handle this correctly. In the second case, NEXTVAL
is a function, called with a string.
Now in parse_func.c, we convert 'AA' to lower to try and find the
sequence table. My assumption is that we should attempt to find the
table without doing a lower(), and if that fails, try lower.
Does that make sense to people. We can't just lower it in every case.

That would create an ambiguity that is better avoided. I think nextval
ought to duplicate the parser's behavior --- if possible, actually call
the same routine the parser uses for looking up a sequence name.
I suggest that it operate like this:

(1) nextval('AA') operates on sequence aa

AA is lowercased, same as unquoted AA would be by the parser.

(2) nextval('"AA"') operates on sequence AA

Quoted "AA" is treated as AA, same as parser would do it.

This should be fully backward compatible with existing SQL code, since
the existing nextval() code implements case (1). I doubt anyone has
tried putting double quotes into their nextval arguments, so adding
the case (2) behavior shouldn't break anything.

I can do that. It looked kind of strange. Usually they do:

create sequence "Aa";

Because nextval is a function with a parameter, it would be
nextval('"Aa"'). I don't think we want to do this for all function
parameters, but just for nextval.

I can do that.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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

That would create an ambiguity that is better avoided. I think nextval
ought to duplicate the parser's behavior --- if possible, actually call
the same routine the parser uses for looking up a sequence name.
I suggest that it operate like this:

(1) nextval('AA') operates on sequence aa

AA is lowercased, same as unquoted AA would be by the parser.

(2) nextval('"AA"') operates on sequence AA

Quoted "AA" is treated as AA, same as parser would do it.

This should be fully backward compatible with existing SQL code, since
the existing nextval() code implements case (1). I doubt anyone has
tried putting double quotes into their nextval arguments, so adding
the case (2) behavior shouldn't break anything.

Good idea. Done.

test=> select nextval('"Aa"');
nextval
-------
3
(1 row)

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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 wrote:

Good idea. Done.

test=> select nextval('"Aa"');
nextval
-------
3
(1 row)

select currval('"Aa"');

?

Vadim

Bruce Momjian wrote:

Good idea. Done.

test=> select nextval('"Aa"');
nextval
-------
3
(1 row)

select currval('"Aa"');

Someone complained they could not get a sequence named "Aa" because the
code auto-lowercased the nextval parameter. The new code accepts
nextval('"Aa"'), and preserves the case. Other cases are
auto-lowercased.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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 wrote:

select currval('"Aa"');

Someone complained they could not get a sequence named "Aa" because the
code auto-lowercased the nextval parameter. The new code accepts
nextval('"Aa"'), and preserves the case. Other cases are
auto-lowercased.

No, just looked in new code and seems that your changes in
parse_func.c handle currval() and setval() as well, sorry.

Vadim

Bruce Momjian wrote:

select currval('"Aa"');

Someone complained they could not get a sequence named "Aa" because the
code auto-lowercased the nextval parameter. The new code accepts
nextval('"Aa"'), and preserves the case. Other cases are
auto-lowercased.

No, just looked in new code and seems that your changes in
parse_func.c handle currval() and setval() as well, sorry.

I meant that non-double quoted cases are auto-lowered. I didn't realize
I was also dealing with other *val cases. I guess that is good.

I am on IRC now.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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