Thread: plpgsql CASE statement - last version
Hello I found some bugs when I used base_lexer, so I returned back own lexer. It's only little bit longer, but simpler. Regards Pavel Stehule
Attachment
Hello 2008/5/1 Jaime Casanova <systemguards@gmail.com>: > On Sat, Apr 5, 2008 at 6:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: >> Hello >> >> I found some bugs when I used base_lexer, so I returned back own >> lexer. It's only little bit longer, but simpler. >> > > you really compile this one? i get a complain because > read_sql_construct is called with 8 arguments and it should have only > 7.. yes, I did it. 8 arguments are from EXECUTE USING patch http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plpgsql/src/gram.y.diff?r1=1.108;r2=1.109;f=h Pavel . > > + expr = read_sql_construct(',', K_THEN, 0, "THEN", > + "SELECT ", true, true, &tok); > > > gram.y: In function 'plpgsql_yyparse': > gram.y:1697: warning: passing argument 5 of 'read_sql_construct' makes > integer from pointer without a cast > gram.y:1697: warning: passing argument 7 of 'read_sql_construct' makes > pointer from integer without a cast > gram.y:1697: error: too many arguments to function 'read_sql_construct' > > -- > regards, > Jaime Casanova > Soporte de PostgreSQL > Guayaquil - Ecuador > Cel. (593) 087171157 >
On Thu, May 1, 2008 at 7:59 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hello > > 2008/5/1 Jaime Casanova <systemguards@gmail.com>: > > On Sat, Apr 5, 2008 at 6:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > >> Hello > >> > >> I found some bugs when I used base_lexer, so I returned back own > >> lexer. It's only little bit longer, but simpler. > >> > > > > you really compile this one? i get a complain because > > read_sql_construct is called with 8 arguments and it should have only > > 7.. > > yes, I did it. 8 arguments are from EXECUTE USING patch > http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plpgsql/src/gram.y.diff?r1=1.108;r2=1.109;f=h > so, i need to see that patch first? patch that doesn't apply because of changes in files (specially definitions moved to other files, but i haven't checked all the .rej yet) -- regards, Jaime Casanova Soporte de PostgreSQL Guayaquil - Ecuador Cel. (593) 087171157
On Thu, May 1, 2008 at 8:11 AM, Jaime Casanova <systemguards@gmail.com> wrote: > On Thu, May 1, 2008 at 7:59 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > Hello > > > > 2008/5/1 Jaime Casanova <systemguards@gmail.com>: > > > On Sat, Apr 5, 2008 at 6:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > >> Hello > > >> > > >> I found some bugs when I used base_lexer, so I returned back own > > >> lexer. It's only little bit longer, but simpler. > > >> > > > > > > you really compile this one? i get a complain because > > > read_sql_construct is called with 8 arguments and it should have only > > > 7.. > > > > yes, I did it. 8 arguments are from EXECUTE USING patch > > http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plpgsql/src/gram.y.diff?r1=1.108;r2=1.109;f=h > > > > so, i need to see that patch first? patch that doesn't apply because > of changes in files (specially definitions moved to other files, but i > haven't checked all the .rej yet) > sorry, you mean already applied RETURN QUERY, right? i was thinking in pending patch RETURN QUERY EXECUTE... i will check again my files but i'm sure i have updated tu CVS TIP before try your patch -- Atentamente, Jaime Casanova Soporte de PostgreSQL Guayaquil - Ecuador Cel. (593) 087171157
On Thu, May 1, 2008 at 8:14 AM, Jaime Casanova <systemguards@gmail.com> wrote: > > On Thu, May 1, 2008 at 8:11 AM, Jaime Casanova <systemguards@gmail.com> wrote: > > On Thu, May 1, 2008 at 7:59 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > Hello > > > > > > 2008/5/1 Jaime Casanova <systemguards@gmail.com>: > > > > On Sat, Apr 5, 2008 at 6:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > > > >> Hello > > > >> > > > >> I found some bugs when I used base_lexer, so I returned back own > > > >> lexer. It's only little bit longer, but simpler. > > > >> > > > > > > > > you really compile this one? i get a complain because > > > > read_sql_construct is called with 8 arguments and it should have only > > > > 7.. > > > > > > yes, I did it. 8 arguments are from EXECUTE USING patch > > > http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/pl/plpgsql/src/gram.y.diff?r1=1.108;r2=1.109;f=h > > > > > > > so, i need to see that patch first? patch that doesn't apply because > > of changes in files (specially definitions moved to other files, but i > > haven't checked all the .rej yet) > > > > sorry, you mean already applied RETURN QUERY, right? i was thinking in > pending patch RETURN QUERY EXECUTE... i will check again my files but > i'm sure i have updated tu CVS TIP before try your patch > ok, you're right... sorry for the noise... i will try it again -- regards, Jaime Casanova Soporte de PostgreSQL Guayaquil - Ecuador Cel. (593) 087171157
On Sat, Apr 5, 2008 at 6:57 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hello
>
> I found some bugs when I used base_lexer, so I returned back own
> lexer. It's only little bit longer, but simpler.
>
you really compile this one? i get a complain because
read_sql_construct is called with 8 arguments and it should have only
7...
+ expr = read_sql_construct(',', K_THEN, 0, "THEN",
+ "SELECT ", true, true, &tok);
gram.y: In function 'plpgsql_yyparse':
gram.y:1697: warning: passing argument 5 of 'read_sql_construct' makes
integer from pointer without a cast
gram.y:1697: warning: passing argument 7 of 'read_sql_construct' makes
pointer from integer without a cast
gram.y:1697: error: too many arguments to function 'read_sql_construct'
--
regards,
Jaime Casanova
Soporte de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157
Pavel Stehule wrote:
> Hello
>
> I found some bugs when I used base_lexer, so I returned back own
> lexer. It's only little bit longer, but simpler.
Hmm. I don't like having to lex the expressions again. It just doesn't
feel right.
How about taking a completely different strategy, and implement the
CASE-WHEN construct fully natively in plpgsql, instead of trying to
convert it to a single SQL CASE-WHEN expression? It's not a very good
match anyway; you have to do tricks to convert the comma-separated lists
of WHEN expressions to WHEN-THEN clauses, and you can't use the
THEN-clauses as is, but you have to replace them with offsets into the
array. I think implementing the logic in pl_exec.c would lead to cleaner
code.
FWIW, the current approach gives pretty cryptic CONTEXT information in
error messages as well. For example, this pretty simple case-when example:
postgres=# create or replace FUNCTION case_test(int)
returns text as $$
begin
case $1
when 1 then
return 'one';
when 'invalid' then
return 'two';
when 3,4,5 then
return 'three, four or five';
end case;
end;
$$ language plpgsql immutable;
CREATE FUNCTION
gives this pretty hard-to-understand error message:
postgres=# SELECT case_test(1);
ERROR: invalid input syntax for integer: "invalid"
CONTEXT: SQL statement "SELECT CASE $1 WHEN 1 THEN 1 WHEN
'invalid' THEN 2 WHEN 3 THEN 3 WHEN 4 THEN 3 WHEN 5 THEN 3 END "
PL/pgSQL function "case_test" line 2 at unknown
BTW, what does PL/SQL or PSM say about the type-compatibility of the
CASE and the WHENs? We're very lenient in assignments, how should this
behave?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Hello 2008/5/2 Heikki Linnakangas <heikki@enterprisedb.com>: > Pavel Stehule wrote: >> >> Hello >> >> I found some bugs when I used base_lexer, so I returned back own >> lexer. It's only little bit longer, but simpler. > > Hmm. I don't like having to lex the expressions again. It just doesn't feel > right. me too. But when I tested to use base_lexer routines, I had to write similar number of lines and add new separate file http://archives.postgresql.org/pgsql-patches/2008-04/msg00131.php - main problems of this version is enhanced strings, because base_lexer don't forward this information. > > How about taking a completely different strategy, and implement the > CASE-WHEN construct fully natively in plpgsql, instead of trying to convert > it to a single SQL CASE-WHEN expression? It's not a very good match anyway; > you have to do tricks to convert the comma-separated lists of WHEN > expressions to WHEN-THEN clauses, and you can't use the THEN-clauses as is, > but you have to replace them with offsets into the array. I think > implementing the logic in pl_exec.c would lead to cleaner code. > It was first variant. It's simpler for parsing and slower for execution :(. It means more than once expression evaluation and for simple case value casting and comparation. > FWIW, the current approach gives pretty cryptic CONTEXT information in error > messages as well. For example, this pretty simple case-when example: > > postgres=# create or replace FUNCTION case_test(int) > returns text as $$ > begin > case $1 > when 1 then > return 'one'; > when 'invalid' then > return 'two'; > when 3,4,5 then > return 'three, four or five'; > end case; > end; > $$ language plpgsql immutable; > CREATE FUNCTION > > gives this pretty hard-to-understand error message: > > postgres=# SELECT case_test(1); > ERROR: invalid input syntax for integer: "invalid" > CONTEXT: SQL statement "SELECT CASE $1 WHEN 1 THEN 1 WHEN 'invalid' > THEN 2 WHEN 3 THEN 3 WHEN 4 THEN 3 WHEN 5 THEN 3 END " > PL/pgSQL function "case_test" line 2 at unknown > > BTW, what does PL/SQL or PSM say about the type-compatibility of the CASE > and the WHENs? We're very lenient in assignments, how should this behave? > what I know, it have to be compareable case $1 when 1 then return 'one'; when 'invalid' then return 'two'; is same as if expr1 = 1 then\ return 'one' elsif expr1 = 'invalid' then .... I have to agree, so this message is ugly, but on second hand I can check this error in compile time. When I move it to pl_exec I can detect this bug much later, so current solution is safe. So, when I read error message I see correct message - only CONTEXT is crazy (but it is strange not only in this case)/ I haven't idea how I can solve it - maybe notice in documentation. We can add hint to this message. Other way (mix read_sql_construct and add_expr) has other problem with nested case statements. Maybe safe to somewhere positions of placeholders. But It will not be simple too, because width of placeholders can be different. Regards Pavel Stehule > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com >
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> 2008/5/2 Heikki Linnakangas <heikki@enterprisedb.com>:
>> How about taking a completely different strategy, and implement the
>> CASE-WHEN construct fully natively in plpgsql, instead of trying to convert
>> it to a single SQL CASE-WHEN expression? It's not a very good match anyway;
> It was first variant. It's simpler for parsing and slower for
> execution :(. It means more than once expression evaluation and for
> simple case value casting and comparation.
I agree with Heikki: this patch is seriously ugly, and "slower for
execution" isn't a good enough reason for saddling us with having
to maintain such a kluge in the parser.
I don't really see why you should need to have multiple expression
evaluations, anyhow. Can't you evaluate the test expression once
and inject its value into the comparisons using CaseTestExpr,
the same way the core CASE-expression code works?
regards, tom lane
Hello
2008/5/3 Tom Lane <tgl@sss.pgh.pa.us>:
> "Pavel Stehule" <pavel.stehule@gmail.com> writes:
>> 2008/5/2 Heikki Linnakangas <heikki@enterprisedb.com>:
>>> How about taking a completely different strategy, and implement the
>>> CASE-WHEN construct fully natively in plpgsql, instead of trying to convert
>>> it to a single SQL CASE-WHEN expression? It's not a very good match anyway;
>
>> It was first variant. It's simpler for parsing and slower for
>> execution :(. It means more than once expression evaluation and for
>> simple case value casting and comparation.
>
> I agree with Heikki: this patch is seriously ugly, and "slower for
> execution" isn't a good enough reason for saddling us with having
> to maintain such a kluge in the parser.
>
> I don't really see why you should need to have multiple expression
> evaluations, anyhow. Can't you evaluate the test expression once
> and inject its value into the comparisons using CaseTestExpr,
> the same way the core CASE-expression code works?
>
>
I have to look on this way.
Regards
Pavel Stehule
regards, tom lane
>
Hello I am sending little bit smarter version - without redundant parsing. When test expression is defined, then expressions in WHEN part are modified like $x in ( origin_expression ) $x is referenced to invisible *case* variable that carries result of test expression. Regards Pavel Stehule 2008/5/3 Pavel Stehule <pavel.stehule@gmail.com>: > Hello > > 2008/5/3 Tom Lane <tgl@sss.pgh.pa.us>: >> "Pavel Stehule" <pavel.stehule@gmail.com> writes: >>> 2008/5/2 Heikki Linnakangas <heikki@enterprisedb.com>: >>>> How about taking a completely different strategy, and implement the >>>> CASE-WHEN construct fully natively in plpgsql, instead of trying to convert >>>> it to a single SQL CASE-WHEN expression? It's not a very good match anyway; >> >>> It was first variant. It's simpler for parsing and slower for >>> execution :(. It means more than once expression evaluation and for >>> simple case value casting and comparation. >> >> I agree with Heikki: this patch is seriously ugly, and "slower for >> execution" isn't a good enough reason for saddling us with having >> to maintain such a kluge in the parser. >> >> I don't really see why you should need to have multiple expression >> evaluations, anyhow. Can't you evaluate the test expression once >> and inject its value into the comparisons using CaseTestExpr, >> the same way the core CASE-expression code works? >> >> > > I have to look on this way. > > Regards > Pavel Stehule > > regards, tom lane >> >
Attachment
"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> I am sending little bit smarter version - without redundant parsing.
Applied with corrections --- you had some memory management problems
in particular.
One thing that I think might annoy people is that you've handled
CASE x
WHEN a, b, c THEN ...
by producing the equivalent of "IF x IN (a, b, c)". This means that
all three of the a, b, c expressions will be evaluated even if "a"
matches. The SQL spec doesn't appear to promise short-circuit
evaluation in such a case, but I suspect somebody out there might
have a problem someday. It didn't seem tremendously easy to fix though.
I suppose anyone who does have a problem can rewrite as
CASE x
WHEN a THEN ...
WHEN b THEN ...
WHEN c THEN ...
at the cost of duplicating their THEN code.
regards, tom lane