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

Allow to change autocommit attribute #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

presidento
Copy link

If autocommit changed, it was not propagated to SqliteMultithread, since its autocommit value was set only in initialization phase. According to the documentation we can temporarily disable autocommit if we need performance, but the performance was still the same as with keeping autocommit enabled.

Test case 1

If autocommit is False, then the dict should not be saved if we forget to call commit. (This is the case if we initialize SqliteDict with autocommit=False)

my_dict = sqlitedict.SqliteDict(filename="autocommit-test.sqlite", autocommit=True)
my_dict.autocommit = False
my_dict["a"] = 12
del my_dict

my_dict = sqlitedict.SqliteDict(filename="autocommit-test.sqlite")
print("a" in my_dict) # Should be False, but it is True (commit was called)
my_dict.terminate()

Test case 2

Performance measusements with this file:

import sqlitedict
import time
import contextlib

@contextlib.contextmanager
def measure(name):
    start = time.time()
    yield
    end = time.time()
    print(f"Elapsed for {name}: {end-start:.1f} seconds")

def fill_values(in_dict):
    for index in range(1_000):
        in_dict[index] = index

with measure("autocommit=False -> True"):
    my_dict = sqlitedict.SqliteDict(filename="mytest.sqlite", autocommit=False)
    my_dict.autocommit = True
    fill_values(my_dict)
    my_dict.commit()
    my_dict.terminate()

with measure("autocommit=True"):
    my_dict = sqlitedict.SqliteDict(filename="mytest.sqlite", autocommit=True)
    fill_values(my_dict)
    my_dict.commit()
    my_dict.terminate()

with measure("autocommit=False"):
    my_dict = sqlitedict.SqliteDict(filename="mytest.sqlite", autocommit=False)
    fill_values(my_dict)
    my_dict.commit()
    my_dict.terminate()

with measure("autocommit=True -> False"):
    my_dict = sqlitedict.SqliteDict(filename="mytest.sqlite", autocommit=True)
    my_dict.autocommit = False
    fill_values(my_dict)
    my_dict.commit()
    my_dict.terminate()

Results:

# Original
❯ python .\sqlitedict-test.py
Elapsed for autocommit=False -> True: 1.9 seconds
Elapsed for autocommit=True: 2.1 seconds
Elapsed for autocommit=False: 0.1 seconds
Elapsed for autocommit=True -> False: 1.5 seconds # <------

# Fixed version
❯ python .\sqlitedict-test.py
Elapsed for autocommit=False -> True: 1.9 seconds
Elapsed for autocommit=True: 2.2 seconds
Elapsed for autocommit=False: 0.1 seconds
Elapsed for autocommit=True -> False: 0.2 seconds # <------

You can be inspired with this pull request and reject this one, or I may can change it to follow the contributing guides. For me using WAL journal mode with autocommit was a perfect solution, I just wanted to let you know that it did not work as I expected.

If autocommit changed, it was not propagated to SqliteMultithread,
since its autocommit value was set only in initialization phase.
According to the documentation we can temporarily disable autocommit
if we need performance, but the performance was still the same as
with keeping autocommit enabled.
@mpenkov
Copy link
Collaborator

mpenkov commented Mar 30, 2023

Sorry for the late reply. Looks good, but is it possible to add tests?

@presidento
Copy link
Author

Sorry, I moved forward. You can consider this as a bug report with a hint how to fix.

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.

3 participants