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

fix: Fix errors on transaction status #464

Merged
merged 12 commits into from
Oct 30, 2023
Merged

Conversation

bpvgoncalves
Copy link
Contributor

@bpvgoncalves bpvgoncalves commented Jun 13, 2023

Some packages dependant on RSQLite are applying commit/rollback when no open transactions exist. Raising this error seems to be breaking some of them.

@krlmlr
Copy link
Member

krlmlr commented Jun 13, 2023

Thanks, good catch. I wonder if we should rather fail issues downstream and release nevertheless. Calling dbCommit() without a transaction should be an error, I wonder why DBItest doesn't test it.

@bpvgoncalves
Copy link
Contributor Author

I can conceive some reasonable use cases using dbRollback() to try to revert changes to a previous state if some unhandled error occurs, but not really for the dbCommit().

Thinking out loud, maybe a this point replacing the error by a message/warning is a compromise solution and lends them some time to make the necessary changes.

@krlmlr
Copy link
Member

krlmlr commented Jun 13, 2023

A warning sounds about right to me. Thanks!

@bpvgoncalves
Copy link
Contributor Author

bpvgoncalves commented Jun 13, 2023

Calling dbCommit() without a transaction should be an error, I wonder why DBItest doesn't test it.

This sounded quite weird for me, and I decided to take a deeper look at their code.
My mistake was I was assuming every dbCommit() or dbRollback() must have a matching dbBegin(). In this case this is not true. They are initiating a transaction by issuing a "BEGIN IMMEDIATE" command via dbExecute and then using dbCommit/dbRollback to close it. This will not trigger an increment on the transaction counter and will then fail when trying to close it.

Let me think a bit more about this to try to find an alternative implementation.

@bpvgoncalves
Copy link
Contributor Author

Let me think a bit more about this to try to find an alternative implementation.

So, I just completely reworked this. Now we're asking for the transaction status directly to the backend instead of trying to maintain a counter of open transactions. It is much more simple and should also be way more robust.
I do apologize the previous iteration didn't work as intended.

@bpvgoncalves bpvgoncalves marked this pull request as ready for review June 14, 2023 19:19
@bpvgoncalves bpvgoncalves changed the title Remove error on no transactions left Fix errors on transaction status Jun 14, 2023
Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I wonder if we should test for autocommit mode instead:

@bpvgoncalves
Copy link
Contributor Author

bpvgoncalves commented Jun 19, 2023

Thanks. I wonder if we should test for autocommit mode instead:

As far as I can see this has the major advantage of not delaying flagging a transaction until a read/write is attempted. Let me take a look at the code, it should not be hard to change the logic to look for autocomnit=off instead of transaction=on.

@krlmlr
Copy link
Member

krlmlr commented Oct 28, 2023

I sent RSQLite 2.3.2 to CRAN, without the new functions. Took me four months to come up with that path of action...

Anyway, once released, we can figure out what to do here, and submit an update.

@bpvgoncalves
Copy link
Contributor Author

Hello. I'm really sorry and sad the initial implementation didn't quite work as intended and the additional workload it might have caused to you. Please let me know if there is something I can do about it.
I trust the current approach of checking for the autocommit status is much more robust and less error prone than the transaction counters used before.

@krlmlr krlmlr changed the title Fix errors on transaction status fix: Fix errors on transaction status Oct 30, 2023
@krlmlr krlmlr merged commit af98957 into r-dbi:main Oct 30, 2023
15 checks passed
@krlmlr
Copy link
Member

krlmlr commented Oct 30, 2023

Thanks, no worries. I like the new approach. We can release the update on November 4, the advantage is that we have a CRAN version with SQLite 3.43. I'll run revdepchecks just to be sure.

@bpvgoncalves bpvgoncalves deleted the fix_brk_rev_dep branch January 2, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants