Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
GitMensch opened this issue Aug 30, 2022 · 10 comments
Labels
bug Something isn't working

Comments

@GitMensch
Copy link
Contributor

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
Copy link
Contributor Author

if (is_commit_or_rollback_statement(query)) {
cursor_manager.clearConnectionCursors(conn->getId(), false);
if (conn->getConnectionOptions()->autocommit) {
rc = dbi->end_transaction(query);
FAIL_ON_ERROR(rc, st, dbi, DBERR_END_TX_FAILED)
rc = dbi->begin_transaction();
FAIL_ON_ERROR(rc, st, dbi, DBERR_BEGIN_TX_FAILED)
}
else {
rc = dbi->exec(query);
FAIL_ON_ERROR(rc, st, dbi, DBERR_SQL_ERROR)
}
}
else {
rc = dbi->exec(query);
FAIL_ON_ERROR(rc, st, dbi, DBERR_SQL_ERROR)
if (is_dml_statement(query) && conn->getConnectionOptions()->autocommit) {
rc = dbi->end_transaction("COMMIT");
FAIL_ON_ERROR(rc, st, dbi, DBERR_END_TX_FAILED)
rc = dbi->begin_transaction();
FAIL_ON_ERROR(rc, st, dbi, DBERR_BEGIN_TX_FAILED)
}
}

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
Copy link
Owner

mridoni commented Aug 30, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

mridoni commented Aug 31, 2022

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
Copy link
Owner

mridoni commented Oct 19, 2022

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

mridoni added a commit that referenced this issue Oct 27, 2022
- Changed cursor initialization (should fix #88)
- Fix for "1.0.17/1.0.18 regression: COMMIT and ROLLBACK delete all cursor definitions -> Status -111" (#119)
- Merged pull request #127 (thanks to Simon Sobisch)
@mridoni mridoni added the bug Something isn't working label Nov 18, 2022
@GitMensch
Copy link
Contributor Author

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
Copy link
Contributor Author

friendly ping @mridoni

@mridoni
Copy link
Owner

mridoni commented Dec 22, 2022

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 😄)

@GitMensch
Copy link
Contributor Author

GitMensch commented Dec 22, 2022 via email

@mridoni
Copy link
Owner

mridoni commented Dec 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants