Обсуждение: ecpg preprocessor regression in 9.0
This used to work in the PostgreSQL 8.4 ecpg preprocessor: EXEC SQL EXECUTE mystmt USING 1.23; but in 9.0 it throws an error: floattest.pgc:39: ERROR: variable "1" is not declared Attached is the full test case, drop it in src/interfaces/ecpg/test/preproc and compile. I bisected the cause to this commit: commit b2bddc2ff22f0c3d54671e43c67a2563deed7908 Author: Michael Meskes <meskes@postgresql.org> Date: Thu Apr 1 08:41:01 2010 +0000 Applied Zoltan's patch to make ecpg spit out warnings if a local variable hides a global one with the same name. I don't immediately see why that's causing it, but it doesn't seem intentional. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
On 01.11.2010 15:31, Heikki Linnakangas wrote: > This used to work in the PostgreSQL 8.4 ecpg preprocessor: > > EXEC SQL EXECUTE mystmt USING 1.23; > > but in 9.0 it throws an error: > > floattest.pgc:39: ERROR: variable "1" is not declared > > Attached is the full test case, drop it in > src/interfaces/ecpg/test/preproc and compile. > > I bisected the cause to this commit: > > commit b2bddc2ff22f0c3d54671e43c67a2563deed7908 > Author: Michael Meskes <meskes@postgresql.org> > Date: Thu Apr 1 08:41:01 2010 +0000 > > Applied Zoltan's patch to make ecpg spit out warnings if a local > variable hides a global one with the same name. > > I don't immediately see why that's causing it, but it doesn't seem > intentional. On closer look, it's quite obvious: the code added to ECPGdump_a_type thinks that ECPGt_const is a variable type, and tries to look up the variable. The straightforward fix is this: diff --git a/src/interfaces/ecpg/preproc/type.c b/src/interfaces/ecpg/preproc/type.c index cc668a2..a53018b 100644 --- a/src/interfaces/ecpg/preproc/type.c +++ b/src/interfaces/ecpg/preproc/type.c @@ -246,7 +246,7 @@ ECPGdump_a_type(FILE *o, const char *name, struct ECPGtype * type, struct variable *var; if (type->type != ECPGt_descriptor && type->type != ECPGt_sqlda && - type->type != ECPGt_char_variable && + type->type != ECPGt_char_variable && type->type != ECPGt_const && brace_level >= 0) { char *str; But I wonder if there is a better way to identify variable-kind of ECPGttypes than list the ones that are not. There's some special ECPGttypes still missing from the above if-test, like ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to ECPGdump_a_type. Seems a bit fragile anyway. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Nov 01, 2010 at 03:47:04PM +0200, Heikki Linnakangas wrote: > On closer look, it's quite obvious: the code added to > ECPGdump_a_type thinks that ECPGt_const is a variable type, and > tries to look up the variable. The straightforward fix is this: > ... > But I wonder if there is a better way to identify variable-kind of > ECPGttypes than list the ones that are not. There's some special > ECPGttypes still missing from the above if-test, like > ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to > ECPGdump_a_type. Seems a bit fragile anyway. I agree. How about adding a macro definition explicitely checking for the r= eal variable types? Michael --=20 Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! For=C3=A7a Bar=C3=A7a! Go SF 49ers! Use Debian GNU/Linux, Pos= tgreSQL
>> On closer look, it's quite obvious: the code added to >> ECPGdump_a_type thinks that ECPGt_const is a variable type, and >> tries to look up the variable. The straightforward fix is this: >> ... >> But I wonder if there is a better way to identify variable-kind of >> ECPGttypes than list the ones that are not. There's some special >> ECPGttypes still missing from the above if-test, like >> ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to >> ECPGdump_a_type. Seems a bit fragile anyway. > > I agree. How about adding a macro definition explicitely checking > for the real > variable types? Why not a function instead of a macro? You can set a breakpoint on a function if you need to debug. It seems unlikely that you would every see any measurable performance gain by writing a macro instead of a function. -- Korry
On 02.11.2010 19:39, Michael Meskes wrote: > On Mon, Nov 01, 2010 at 03:47:04PM +0200, Heikki Linnakangas wrote: >> On closer look, it's quite obvious: the code added to >> ECPGdump_a_type thinks that ECPGt_const is a variable type, and >> tries to look up the variable. The straightforward fix is this: >> ... >> But I wonder if there is a better way to identify variable-kind of >> ECPGttypes than list the ones that are not. There's some special >> ECPGttypes still missing from the above if-test, like >> ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to >> ECPGdump_a_type. Seems a bit fragile anyway. > > I agree. How about adding a macro definition explicitely checking for the real > variable types? You'd still have to list them all in the macro, but it would definitely be better. The list would then be closer to the enums, in the same header file, making it easier to remember to keep it up-to-date. (Korry's suggestion of making it a function instead of a macro would not have that advantage). -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
>>> On closer look, it's quite obvious: the code added to >>> ECPGdump_a_type thinks that ECPGt_const is a variable type, and >>> tries to look up the variable. The straightforward fix is this: >>> ... >>> But I wonder if there is a better way to identify variable-kind of >>> ECPGttypes than list the ones that are not. There's some special >>> ECPGttypes still missing from the above if-test, like >>> ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to >>> ECPGdump_a_type. Seems a bit fragile anyway. >> >> I agree. How about adding a macro definition explicitely checking >> for the real >> variable types? > > You'd still have to list them all in the macro, but it would > definitely be better. The list would then be closer to the enums, in > the same header file, making it easier to remember to keep it up-to- > date. (Korry's suggestion of making it a function instead of a macro > would not have that advantage). Sure it would... use a switch statement that explicitly handles each type and include a default case that throws an error (or assertion). Of course, you have to run into the code to find out that you forgot to maintain the classification function. A good C compiler will even spot the problem if you forget to handle one or more enum values in the switch statement. A macro offers no assurances at all. -- Korry
On 01.11.2010 15:47, Heikki Linnakangas wrote: > On 01.11.2010 15:31, Heikki Linnakangas wrote: >> This used to work in the PostgreSQL 8.4 ecpg preprocessor: >> >> EXEC SQL EXECUTE mystmt USING 1.23; >> >> but in 9.0 it throws an error: >> >> floattest.pgc:39: ERROR: variable "1" is not declared >> >> Attached is the full test case, drop it in >> src/interfaces/ecpg/test/preproc and compile. >> >> I bisected the cause to this commit: >> >> commit b2bddc2ff22f0c3d54671e43c67a2563deed7908 >> Author: Michael Meskes <meskes@postgresql.org> >> Date: Thu Apr 1 08:41:01 2010 +0000 >> >> Applied Zoltan's patch to make ecpg spit out warnings if a local >> variable hides a global one with the same name. >> >> I don't immediately see why that's causing it, but it doesn't seem >> intentional. > > On closer look, it's quite obvious: the code added to ECPGdump_a_type > thinks that ECPGt_const is a variable type, and tries to look up the > variable. The straightforward fix is this: > > diff --git a/src/interfaces/ecpg/preproc/type.c > b/src/interfaces/ecpg/preproc/type.c > index cc668a2..a53018b 100644 > --- a/src/interfaces/ecpg/preproc/type.c > +++ b/src/interfaces/ecpg/preproc/type.c > @@ -246,7 +246,7 @@ ECPGdump_a_type(FILE *o, const char *name, struct > ECPGtype * type, > struct variable *var; > > if (type->type != ECPGt_descriptor && type->type != ECPGt_sqlda && > - type->type != ECPGt_char_variable && > + type->type != ECPGt_char_variable && type->type != ECPGt_const && > brace_level >= 0) > { > char *str; > > But I wonder if there is a better way to identify variable-kind of > ECPGttypes than list the ones that are not. There's some special > ECPGttypes still missing from the above if-test, like > ECPGt_NO_INDICATOR, but I'm not sure if they can ever be passed to > ECPGdump_a_type. Seems a bit fragile anyway. This really needs to be fixed, so I just committed the above patch. The code needs some love, it took me a while to realize that find_variable() modifies its argument, for example, which explains the strdups() in ECPGdump_a_type(). And I wonder why we bother to put constants to the global all_variables list at all. And I'm not sure the above type-checks still cover everything. But at least the immediate bug has now been fixed. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com