ECPG patch 4.1, out-of-scope cursor support in nativemode
in [PgSql]
|
Prev: [HACKERS] Bug ? different behaviour between 8.3 and 8.4 won IS NULL with subarrays of nulls
Next: [HACKERS] Helping the CommitFest re PL/Perl changes
From: Boszormenyi Zoltan on 19 Jan 2010 12:45 Michael Meskes írta: > Zoltan, > > while testing your patch I went through the test cases and found this in outofscope.pgc: > > >> + #include <inttypes.h> >> > > As we know by now this won't work. :-) > Okay, I will fix it. :-) I forgot it's in there as well. > Besides, would you mind simplifying the test case a little bit? There is no > need to have it test all the sqlda stuff, too. I don't mind having two cases > testing sqlda but this makes testing out of scope variable handling more > difficult especially with sqlda being a rather complex system with different > struct definitions and so on. > Okay. > Also I wonder why you added struct.pgc. It seems to build and run without a > problem on a non-patched system. Is it an additional test that just happened to > be included here? > It was included in there because the first symptom that lead to this patch under Informix compat mode that a simple struct variable wasn't properly unrolled because adjust_informix() lost informiation about the type. Did I accidentally move it to native mode? I'll move it back. I will send an updated patch. Thanks, 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Boszormenyi Zoltan on 22 Jan 2010 12:11 Michael Meskes írta: >> diff -dcrpN pgsql.orig/src/interfaces/ecpg/test/expected/compat_informix-struct.c pgsql.4.1/src/interfaces/ecpg/test/expected/compat_informix-struct.c >> ... >> + /* Test DECLARE ... SELECT ... INTO with struct type */ >> + >> + ECPGset_var( 0, &( myvar ), __LINE__);\ >> + ECPGset_var( 1, &( mynullvar ), __LINE__);\ >> + ECPG_informix_reset_sqlca(); /* declare mycur cursor for select * from a1 */ >> + #line 45 "struct.pgc" >> + >> + { ECPGdo(__LINE__, 1, 1, NULL, 0, ECPGst_normal, "declare mycur cursor for select * from a1", ECPGt_EOIT, >> + ECPGt_int,&(myvar.id),(long)1,(long)1,sizeof(int), >> ... >> > > Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? > Because there's no ECPGget_var()s emitted for - global variables - variables in the same function ECPGget_var() is only used in case the cursor declaration used INTO/USING and it's in a different function from the one where OPEN/FETCH/CLOSE reside. But this cannot be determined easily, e.g. short of making ECPG a two-pass precompiler, so ECPGset_var() is always used. 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Boszormenyi Zoltan on 24 Jan 2010 13:25 Michael Meskes írta: > On Fri, Jan 22, 2010 at 06:11:51PM +0100, Boszormenyi Zoltan wrote: > >>> Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? >>> >>> >> Because there's no ECPGget_var()s emitted for >> - global variables >> - variables in the same function >> >> ECPGget_var() is only used in case the cursor declaration >> used INTO/USING and it's in a different function from >> the one where OPEN/FETCH/CLOSE reside. But this >> cannot be determined easily, e.g. short of making ECPG >> a two-pass precompiler, so ECPGset_var() is always used. >> > > This shows some well thought implementation. Thanks. But the truth is that your comment about "this adjust_informix() hack goes into native mode only over my dead body" made me come up with this change and the reasoning. :-) Thus if some code with cursors does everything in one function then only some useless ECPGset_var() calls are added by the preprocessor, making the change the least intrusive for old regression tests. > But what I was wondering about was > why this is used as test case. While I see the need to test this part of ecpg I > thought this test case was added because it showed a problem without your > changes. > The problem that popped up first was that adjust_informix() didn't properly deal with structs, remember? I tried some small changes to fix that but they turned out to be improper ones. The small changes worked for the initial problem, i.e. IIRC in some cases adjust_informix() was bypassed and the struct/union members were unrolled properly for Informix compat mode, too. The regression test "compat_informix/struct.pgc" shows this case. Then our client showed a much larger programme which actually used cursors in an event driver way. E.g. PgUp/PgDn called different functions that contained only FETCH NEXT or FETCH PRIOR. This was a curses based terminal programme and my first little patch that solved only the struct unrolling case failed in this case, because either ECPG segfaulted in adjust_informix() or the code that ECPG produced for the FETCH statements didn't have the proper output variables. This is lead to the changes in the patch, which can be summarized as: 1. track function names via the lexer (to minimize the impact of ECPGget_var(), now we can compare the function name where the cursor was DECLAREd and is used) 2. track type names for structs/unions to be able to emit (* ( typename *) &var) in adjust_informix(), and 3. rewriting adjust_informix() to handle struct/union The result is that cursors handling statements (OPEN/FETCH/MOVE) can now be safely put into a different function from where it was DECLAREd. And this makes possible to use ECPG in event driven code, which is a feature that I think worth considering making available in native mode, too. This is why adjust_informix() was also renamed. And while I agree with your comment about that "this can lead to dangerous programming", I think the only thing needed is to document the safety borders, not to prevent crossing them. C is about "living la vida loca". ;-) The regression test introduced (preproc/outofscope.pgc) tries to show this functionality. Currently (without the patch) this test makes ECPG abort in ecpg_type_name(). 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Boszormenyi Zoltan on 25 Jan 2010 13:52 Michael Meskes írta: > On Sun, Jan 24, 2010 at 07:25:24PM +0100, Boszormenyi Zoltan wrote: > >> The problem that popped up first was that adjust_informix() >> didn't properly deal with structs, remember? I tried some >> > > Yes, that's what made me wondering. > > >> i.e. IIRC in some cases adjust_informix() was bypassed >> and the struct/union members were unrolled properly >> for Informix compat mode, too. The regression test >> "compat_informix/struct.pgc" shows this case. Then our >> > > Now this is what I don't get. I may have been unclear. My first attempts at solving it either basically bypassed adjust_informix() or tried to unroll the structs *before* calling adjust_informix(). IIRC, your comment about that solution was something like "unrolling should be done at only one place centrally". Which I agreed after learning what dump_variables() and ECPGdump_a_type() do. > Shouldn't this test use different functions to > really show this problem? I don't think so. The problem only happened in case of cursors because this was the only case when adjust_informix() was called and the lack of handling struct/union there was the problem. The final "else" branch used ecpg_type_name() which calls abort() in case of unknown types. > Isn't it hidden now by the new functionality of not > spitting out ECPGget_var's? > No. This problem is hidden by the fact the adjust_informix() (now adjust_outofscope_cursor_vars()) now handles structs/unions properly and the struct members are properly unrolled by ECPGdump_a_type(). The "not spitting out ECPGget_var()" part is about tidying the preprocessed C source. As I wrote previously, globals don't need transformation and nor the locals in case the OPEN/FETCH statements are in the same function as DECLARE. But considering all the above, we might not need the new compat_informix/struct.pgc regression test. I think it was tested already in e.g. preproc/array_of_struct.pgc and preproc/type.pgc and the new feature (if accepted) is a unified one, it would show the problem for native mode as well. 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Boszormenyi Zoltan on 25 Jan 2010 14:17
Michael Meskes írta: > On Mon, Jan 25, 2010 at 07:52:05PM +0100, Boszormenyi Zoltan wrote: > >> But considering all the above, we might not need the new >> compat_informix/struct.pgc regression test. I think it was tested >> already in e.g. preproc/array_of_struct.pgc and preproc/type.pgc >> and the new feature (if accepted) is a unified one, it would show >> the problem for native mode as well. >> > > That was my feeling too and the reason for these questions. Again, this has > nothing to do with the feature you implemented, it was just about this special > test. > Should I send you a new patch without this regression test or do you delete it before applying the patch? BTW, thank you very much for the review. 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |