mridoni / gixsql

GixSQL is an ESQL preprocessor and a series of runtime libraries to enable GnuCOBOL to access PostgreSQL, ODBC, MySQL, Oracle and SQLite databases.
GNU General Public License v3.0
16 stars 8 forks source link

1.0.17/1.0.18 regression: `COMMIT` and `ROLLBACK` delete all cursor definitions -> Status -111 #119

Open GitMensch opened 2 years ago

GitMensch commented 2 years ago

The main question is: is that correct, but I'm quite sure it isn't.

The current code leads to:

PROG1 -> PROG2 (OPEN + READ CURSOR2 + CLOSE) PROG1 -> PROG3 (OPEN + READ CURSOR3 + CLOSE) PROG1 -> PROG4 (OPEN + READ CURSOR4 + CLOSE) PROG1 COMMIT PROG1 -> PROG2 (OPEN CURSOR2) -> returns sqlcode -111, sqlstat "24518" sqlmessage "No such cursor"

each of PROG2-4 executes a cursor open which automatically declares the cursor internally because "GIXSQL-CI-F-PROGn-CSKEY01N = ' ' ) but the COMMIT executed via GIXSQLExec->_gixsqlExec goes to CursorManager::clearConnectionCursors which removes the definition of every known cursor via CursorManager::remove.

I think what would be reasonable is to close the cursors, but not remove the internal definition... and I think that this issue should actually should postpone 1.0.18a :-(

GitMensch commented 2 years ago

https://github.com/mridoni/gixsql/blob/b907882ea31a0d09430de5182032760e9b4c34aa/runtime/libgixsql/gixsql.cpp#L293-L319 commenting line 294 leads to "program runs again" (like it did with a previous version of GixSQL) - but I haven't checked the implications and possible issues of doing so.

mridoni commented 2 years ago

From what I can see, you are probably right. This part will be overhauled anyway when tackling the "autocommit" stuff (this is why COMMIT and ROLLBACK are being intercepted). I am scheduling this work as the centerpiece of v1.0.19, together with updatable cursors.

GitMensch commented 2 years ago

What do you think about working with milestones on GitHub to schedule and group issues and give some "progress" people can see? [feel totally free to say "no thanks" :-) ]

mridoni commented 2 years ago

Yes, that would be nice :-) Starting in these days, and in the next few weeks, I'll be busier with my day-job and - in the end of September/first half of October - some vacation, but when I get back, I'd like to tackle this and give some more "structure" to the project. This will also encompass Gix-IDE: the new debugger is almost ready for launch, even if in a pre-release form, and this will help speed up the development of the IDE itself.

mridoni commented 2 years ago

While waiting for the "autocommit overhaul" in 1.0.19, this patch might fix this. I have developed a test case (included below) just in a single program (no CALLed subprograms) that passes with no regressions in PostgreSQL with emulated cursors. In order to stay open after a COMMIT or ROLLBACK, the cursors must be declared WITH HOLD as per the ProCOBOL docs (and DB2).

p119.patch.txt TSQL035A.cbl.txt

Edit: due to a mistake the patchincludes the fix in PR #127, just skip the block if you already applied it locally

GitMensch commented 1 year ago

Should this be solved in 1.0.19?

I get a SIGSEGV on cursor close (on delete dbi_data;) and am not sure if it is related:

#0  0x000000000174f010 in ?? ()
#1  0x00007fdb7da2fecd in Cursor::setPrivateData (this=0x185eed0, d=0x0) at ../../../runtime/libgixsql/Cursor.cpp:200
#2  0x00007fdb74838aac in DbInterfacePGSQL::close_cursor (this=0x17a0a50, cursor=0x185eed0) at ../../../runtime/libgixsql-pgsql/DbInterfacePGSQL.cpp:528
#3  0x00007fdb7da70eb5 in GIXSQLCursorClose (st=0x7fdb59bdf7c0 <b_53>, cname=0x7fdb59953ab2 "COBOL_CRS_KEY01NG") at ../../../runtime/libgixsql/gixsql.cpp:916

likely happens because that cursor data is not valid anymore.

GitMensch commented 1 year ago

friendly ping @mridoni

mridoni commented 1 year ago

Yes, this should be solved in v1.0.19, that part has been modified to accomodate for the new autocommit support. During the holidays I am going to perform some triage and close some stale issues, this is probably needed to keep a clear view of what's going on (i guess one can always re-open them :smile:)

GitMensch commented 1 year ago

Ok then we need a new issue for the SIGSEGV from above - as this was seen in 1.0.19

mridoni commented 1 year ago

Opened #135 to track it, but this should disappear in v1.0.20 anyway (including dev versions) since all those allocations are now handled with smart pointers.