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(at)postgresql(dot)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(at)postgresql(dot)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 real
variable types?

Michael
--
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ça Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

>> 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(at)postgresql(dot)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