Problems with variable cursorname in ecpg

Hi,

I did some more testing on ecpg and found that allowing variables as cursor
names seems to produce more problems than I anticipated. But then maybe it's
just some missing checks to throw out error messages. Anyway, I attach a small
test program that, from my understanding, should work, but dosn't. Could
somebody with access to embedded SQL precompilers from other DBMSes please try
if this test case works with them?

The problem we seem to have right now comes from the original logic in ecpg
moving the declare cursor statement to the position of the open cursor
statemend at compile time. With the cursor name being unique this never has
been a problem. However, with variables as cursor names, this uniqueness need
not hold anymore. If it does, i.e. each cursor gets its own variable, all is
well, but if not, it doesn't work correctly at all times.

BTW I can modify the test case so it works fine, but ecpg will still throw an
error message, which is not a good situation either.

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
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes(at)jabber(dot)org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Hi,

Michael Meskes írta:
> Hi,
>
> I did some more testing on ecpg and found that allowing variables as cursor
> names seems to produce more problems than I anticipated. But then maybe it's
> just some missing checks to throw out error messages. Anyway, I attach a small
> test program that, from my understanding, should work, but dosn't. Could
> somebody with access to embedded SQL precompilers from other DBMSes please try
> if this test case works with them?
>

I have modified your code a little to be able to compile with ESQL/C.
Attached both the embedded SQL and the processed source.

> The problem we seem to have right now comes from the original logic in ecpg
> moving the declare cursor statement to the position of the open cursor
> statemend at compile time. With the cursor name being unique this never has
> been a problem. However, with variables as cursor names, this uniqueness need
> not hold anymore. If it does, i.e. each cursor gets its own variable, all is
> well, but if not, it doesn't work correctly at all times.
>

This was what I found some time ago when the same issue, i.e.
two DECLAREs for the same cursor name in IF/ELSE appeared
for different queries:

The standard says (SQL:2008, section 14.1 <declare cursor>,
Syntax Rules):

"
Syntax Rules
1) If a <declare cursor> is contained in an
<SQL-client module definition> M, then:
a) The <cursor name> shall not be equivalent to the
<cursor name> of any other <declare cursor> or
<dynamic declare cursor> in M.
b) The scope of the <cursor name> is M with the exception
of any <SQL schema statement> contained in M.
c) Any <host parameter name> contained in the <cursor specification>
shall be defined in a <host parameter declaration> in the
<externally-invoked procedure> that contains an <open statement>
that specifies the <cursor name> and is contained in the scope of
that <cursor name>.
"

The standard text doesn't say a word about DECLARE has to imply
a function call, ESQL/C does call a function, ECPG doesn't.
Also, in the same section, under General Rules:

"
General Rules
1) A cursor declaration descriptor CDD is created. CDD includes
indications that:
a) The kind of cursor is a standing cursor.
b) The provenance of the cursor is an indication of the SQL-client
module whose <SQL-client module definition> contains the
<declare cursor>.
c) The name of the cursor is the <cursor name>.
d) The cursor's origin is the <cursor specification> contained in
the <declare cursor>.
e) The cursor's declared properties are as determined by the
<cursor properties>.
"

This says "A cursor declaration descriptor CDD is created." - it doesn't
say where, and ECPG treats it as internal descriptor (as opposed to
a runtime descriptor in the processed C code as done by ESQL/C)
and currently it uses it to enforce the rules about cursors in embedded
SQL programs, like (in section 21.1 <embedded SQL host program>):

"
14) A <declare cursor> that is contained in an <embedded SQL host program>
shall precede in the text of that <embedded SQL host program> any
SQL-statement that references the <cursor name> of the <declare cursor>.

15) A <dynamic declare cursor> that is contained in an
<embedded SQL host program> shall precede in the text of that
<embedded SQL host program> any SQL-statement that references the
<cursor name> of the <dynamic declare cursor>.
"

and the paragraph 1)a) cited above in "Syntax Rules" of section 14.1.

The above was described as "the DECLARE statement is declarative"
by You, the ECPG maintainer when I fixed another bug that was reported
by our client. If you remember, the bug was that Informix resets
SQLCA upon executing DECLARE, and PostgreSQL didn't do it because
DECLARE wasn't calling any function, it only have set up the
internal descriptor for the cursor. You accepted a fix for this
for the Informix compatible mode of ECPG but not for the native mode,
to keep the declarative nature of DECLARE. But this didn't change
the fact that DECLARE still doesn't involve any function call that
uses name of the cursor.

The interpretation of the standard in the above way (DECLARE is declarative,
not functional) leads to the situation where the ECPG transformation cannot
know the cursor's real name during runtime (ECPG is not a VM after all),
only at transformation time. This means that with a dynamic cursorname
the only thing it can check and match is the ":variablename" cursorname
so OPEN, FETCH and CLOSE will all mandatorily have to use the same
variable as was used in the DECLARE statement.

BTW, the declarative nature of the DECLARE statement means that
it (a DECLARE statement) can appear outside of any functions
in ECPG's native mode and ESQL/C's failure in (or interpretation of)
conforming to the standard treats it as an error.

The uniqueness problem can only be solved with modifying
the runtime library to keep track of the cursor names in the client.
It would ruin the declarative nature of DECLARE but would increase
compatibility with Informix, and we would also need to implement
correct "FREE cursorname" behaviour, too. Which would also bring
the consequence that the ECPG client library would need to
forbid cursors and prepared statements with the same name as
"FREE" can also free cursors and prepared statements.

But there's a workaround that is usable under ECPG.
One of the cursors can be put into a different source file,
and different statements for cursors (DECLARE, OPEN, FETCH,
CLOSE) can now be put into different functions. You can
even have different cursornames passed into the same
DECLARE using different statements with different number of
input parameters and different output structure and have
it all work using named SQL or SQLDA descriptors.

I think the current behaviour is the best we could achieve
while keeping close standard conformance.

Best regards,
Zoltán Böszörményi

> BTW I can modify the test case so it works fine, but ecpg will still throw an
> error message, which is not a good situation either.
>
> Michael
>
> ------------------------------------------------------------------------
>
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

> The interpretation of the standard in the above way (DECLARE is declarative,
> ...

It's not just interpretation, but also a regression if we were to change this.

> The uniqueness problem can only be solved with modifying
> the runtime library to keep track of the cursor names in the client.
> It would ruin the declarative nature of DECLARE but would increase
> compatibility with Informix, and we would also need to implement
> correct "FREE cursorname" behaviour, too. Which would also bring
> the consequence that the ECPG client library would need to
> forbid cursors and prepared statements with the same name as
> "FREE" can also free cursors and prepared statements.

True.

> I think the current behaviour is the best we could achieve
> while keeping close standard conformance.

I think we should make the error message/documentation a little bit clearer as
people have stumbled over it. Having said that couldn't we keep the statement
declarative only for statements that do not carry a variable? This will not
break any onld program and besides using a variable that doesn't exist, because
you're outside a function doesn't make sense either. This is probably something
for 9.1 though if it indeed works.

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
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes(at)jabber(dot)org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Michael Meskes írta:
>> The interpretation of the standard in the above way (DECLARE is declarative,
>> ...
>>
>
> It's not just interpretation, but also a regression if we were to change this.
>

Obviously.

>> The uniqueness problem can only be solved with modifying
>> the runtime library to keep track of the cursor names in the client.
>> It would ruin the declarative nature of DECLARE but would increase
>> compatibility with Informix, and we would also need to implement
>> correct "FREE cursorname" behaviour, too. Which would also bring
>> the consequence that the ECPG client library would need to
>> forbid cursors and prepared statements with the same name as
>> "FREE" can also free cursors and prepared statements.
>>
>
> True.
>
>
>> I think the current behaviour is the best we could achieve
>> while keeping close standard conformance.
>>
>
> I think we should make the error message/documentation a little bit clearer as
> people have stumbled over it.

Yes, we need to document it.

> Having said that couldn't we keep the statement
> declarative only for statements that do not carry a variable? This will not
> break any onld program and besides using a variable that doesn't exist, because
> you're outside a function doesn't make sense either.

I think you forget that in this case, only global variables are
usable in the DECLARE in this case, no local variables in
functions preceding the DECLARE are visible to it.

What we need here is an extra check in ECPGdump_a_type().
We need to raise an error if
ECPGdump_a_type(name, type, ...)
and
var = with find_variable(name);
on the passed name disagrees in the variable type and maybe
a warning if they disagree in the brace_level. The same applies
to the indicator variable. For that, we need to pass the brace_level
to ECPGdump_a_type() for both the variable and the indicator.

> This is probably something
> for 9.1 though if it indeed works.
>

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Hi,

It would be good if libecpg to support e.g. "char **strings" instead of
"char *strings[]" for preallocated strings and the array of those.
IIRC, the first version overwrote my stack in a test programme, as these
two are treated the same but they are not.

Also, as "numeric" is supposed to be allocated using PGTYPESnumeric_new(),
we would need to support array of pointers to numeric, as well as the
current
static array of numerics. There's no way PGTYPESnumeric_free() works
on a static array, e.g. glibc will abort the app with a debug output of
free() on
an invalid pointer. Freeing the "digit" pointer manually is ugly,
numeric has
accessor functions, after all. Not doing anything leads to a memory leak.
Somehow digitbuf_free() should be make visible outside for such static
numeric variables.

These and other types came up regarding problems with "FETCH N"
in ECPG. All boils down to the same problem: "sometype *ptr" is
not equivalent to "sometype ptr[]" if you use &ptr, which is used
by ECPG in most cases.

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Wed, Mar 31, 2010 at 10:35:31AM +0200, Boszormenyi Zoltan wrote:
> > I think we should make the error message/documentation a little bit clearer as
> > people have stumbled over it.
>
> Yes, we need to document it.

I changed the error message and documented a possible improvement in the TODO list.

> I think you forget that in this case, only global variables are
> usable in the DECLARE in this case, no local variables in
> functions preceding the DECLARE are visible to it.

I thought about not allowing variables in declare statements that are outside a
function. Do you think it makes sense to allow those? Forbidding these right
now would give us more headroomfor future development.

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
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes(at)jabber(dot)org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Wed, Mar 31, 2010 at 10:37:13AM +0200, Boszormenyi Zoltan wrote:
> It would be good if libecpg to support e.g. "char **strings" instead of
> "char *strings[]" for preallocated strings and the array of those.
> ...

I'm open to improvements. The way ecpg accesses variables hasn't been changed
for ages, just new types got added.

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
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes(at)jabber(dot)org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Boszormenyi Zoltan írta:
> I think you forget that in this case, only global variables are
> usable in the DECLARE in this case, no local variables in
> functions preceding the DECLARE are visible to it.
>
> What we need here is an extra check in ECPGdump_a_type().
> We need to raise an error if
> ECPGdump_a_type(name, type, ...)
> and
> var = with find_variable(name);
> on the passed name disagrees in the variable type and maybe
> a warning if they disagree in the brace_level. The same applies
> to the indicator variable. For that, we need to pass the brace_level
> to ECPGdump_a_type() for both the variable and the indicator.
>

I was thinking about something like the attached patch.
It passes all the regression tests.

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Boszormenyi Zoltan írta:
> Boszormenyi Zoltan írta:
>
>> I think you forget that in this case, only global variables are
>> usable in the DECLARE in this case, no local variables in
>> functions preceding the DECLARE are visible to it.
>>
>> What we need here is an extra check in ECPGdump_a_type().
>> We need to raise an error if
>> ECPGdump_a_type(name, type, ...)
>> and
>> var = with find_variable(name);
>> on the passed name disagrees in the variable type and maybe
>> a warning if they disagree in the brace_level. The same applies
>> to the indicator variable. For that, we need to pass the brace_level
>> to ECPGdump_a_type() for both the variable and the indicator.
>>
>>
>
> I was thinking about something like the attached patch.
> It passes all the regression tests.
>

And here's the code to test with.

> Best regards,
> Zoltán Böszörményi
>
>
> ------------------------------------------------------------------------
>
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Michael Meskes írta:
> On Wed, Mar 31, 2010 at 10:35:31AM +0200, Boszormenyi Zoltan wrote:
>
>>> I think we should make the error message/documentation a little bit clearer as
>>> people have stumbled over it.
>>>
>> Yes, we need to document it.
>>
>
> I changed the error message and documented a possible improvement in the TODO list.
>
>
>> I think you forget that in this case, only global variables are
>> usable in the DECLARE in this case, no local variables in
>> functions preceding the DECLARE are visible to it.
>>
>
> I thought about not allowing variables in declare statements that are outside a
> function. Do you think it makes sense to allow those? Forbidding these right
> now would give us more headroomfor future development.
>

I think forbidding global variables in DECLARE is not good.
Consider this code (existing code using this practice is in use):

/* globals for our cursor */
EXEC SQL BEGIN DECLARE SECTION;
char *global_curname;
EXEC SQL END DECLARE SECTION;

void open_cursor(char *curname)
{
global_curname = curname;
EXEC SQL DECLARE :global_curname CURSOR FOR ...;
EXEC SQL OPEN :global_curname;
}

... and similar codes for FETCH (into SQLDA) and CLOSE.
It works nicely for single-threaded code.

The dump_variables() code cannot distinguish between the
dynamic cursorname variable (used for $0) and the other
input variables. Considering the usefulness of the current
state, we shouldn't disallow global variables for DECLARE.

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

Hi,

here's a little beautified patch:
- more logical parameter order in ECPGdump_a_type()
- use mm_strdup() instead of strdup() (I notoriously forget this)
- actually bail out with ET_FATAL if the local variable is
of a different type than the global variable that was used in
the DECLARE in the global scope

Although with this patch we can only detect variables under
DECLARE SECTIONs, so we can't detect the scenario
in the attached test case. Should we? This test code would be
a good candidate for the Underhanded C Contest. :-)

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

On Wed, Mar 31, 2010 at 01:15:20PM +0200, Boszormenyi Zoltan wrote:
> I was thinking about something like the attached patch.
> It passes all the regression tests.

Thanks. Applied.

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
ICQ 179140304, AIM/Yahoo/Skype michaelmeskes, Jabber meskes(at)jabber(dot)org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Hi,

I saw you changed my "warn about hidden vars" patch
to ET_ERROR instead of ET_FATAL. My thinking in using
ET_FATAL was that this is an error level that immedately
quits processing and deletes its output file. However,
you are right that we might want to report all such errors
in one run of ECPG.

But if ET_ERROR is used, the output file stays there as if
there was no error. Think about a project that uses Makefile
with say:
.PRECIOUS: %.c
to keep the generated C sources by ECPG. However, the implicit
thinking is that the files were correctly generated.

In such a case, GNU make won't delete the generated file,
and ECPG won't be called the next time "make" is run as
the *.c file already exists (make won't know that ECPG reported
an error code last time) and the C compiler will be called.
directly. GCC on the other hand, deletes its object file
in case of an error (it also reports as many errors as it can),
so running make next time won't attempt linking to a bad
object file.

Attached is the patch to delete the output file if an error
happened. Comments?

Best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

FYI, I think Michael Meskes applied this patch, though I didn't see you
emailed that it was applied.

---------------------------------------------------------------------------

Boszormenyi Zoltan wrote:
> Hi,
>
> here's a little beautified patch:
> - more logical parameter order in ECPGdump_a_type()
> - use mm_strdup() instead of strdup() (I notoriously forget this)
> - actually bail out with ET_FATAL if the local variable is
> of a different type than the global variable that was used in
> the DECLARE in the global scope
>
> Although with this patch we can only detect variables under
> DECLARE SECTIONs, so we can't detect the scenario
> in the attached test case. Should we? This test code would be
> a good candidate for the Underhanded C Contest. :-)
>
> Best regards,
> Zolt?n B?sz?rm?nyi
>
> --
> Bible has answers for everything. Proof:
> "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
> than these cometh of evil." (Matthew 5:37) - basics of digital technology.
> "May your kingdom come" - superficial description of plate tectonics
>
> ----------------------------------
> Zolt?n B?sz?rm?nyi
> Cybertec Sch?nig & Sch?nig GmbH
> http://www.postgresql.at/
>

[ Attachment, skipping... ]

[ Attachment, skipping... ]

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

Yes, he applied the first version without seeing this one,
then he asked for a re-diff privately.

Bruce Momjian írta:
> FYI, I think Michael Meskes applied this patch, though I didn't see you
> emailed that it was applied.
>
> ---------------------------------------------------------------------------
>
> Boszormenyi Zoltan wrote:
>
>> Hi,
>>
>> here's a little beautified patch:
>> - more logical parameter order in ECPGdump_a_type()
>> - use mm_strdup() instead of strdup() (I notoriously forget this)
>> - actually bail out with ET_FATAL if the local variable is
>> of a different type than the global variable that was used in
>> the DECLARE in the global scope
>>
>> Although with this patch we can only detect variables under
>> DECLARE SECTIONs, so we can't detect the scenario
>> in the attached test case. Should we? This test code would be
>> a good candidate for the Underhanded C Contest. :-)
>>
>> Best regards,
>> Zolt?n B?sz?rm?nyi
>>
>> --
>> Bible has answers for everything. Proof:
>> "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
>> than these cometh of evil." (Matthew 5:37) - basics of digital technology.
>> "May your kingdom come" - superficial description of plate tectonics
>>
>> ----------------------------------
>> Zolt?n B?sz?rm?nyi
>> Cybertec Sch?nig & Sch?nig GmbH
>> http://www.postgresql.at/
>>
>>
>
> [ Attachment, skipping... ]
>
> [ Attachment, skipping... ]
>
>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/