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

SQLite connection options #581

Merged
merged 18 commits into from
Feb 24, 2025
Merged

SQLite connection options #581

merged 18 commits into from
Feb 24, 2025

Conversation

tcely
Copy link
Contributor

@tcely tcely commented Dec 11, 2024

Configure the connection PRAGMA values from settings.

It's now less messy.
However, the DatabaseWrapper is not exactly fun to work with.

I've made changes to hopefully make it as easy to maintain as I can.

tcely added 16 commits December 11, 2024 12:33
The `3.2.x` versions don't have a lot of the code that `5.1.x` uses for this.
Newer Django will set this for us,
but it won't matter if it is disabled twice.
TypeError: 'transaction_mode' is an invalid keyword argument for Connection()
TypeError: 'init_command' is an invalid keyword argument for Connection()
Oops. It helps to have the resulting connection.
A possible future improvement would be to define a map of which keys Connection accepts.

Right now, this is removing keys after Connection fails because of an unknown key.

This could be automated by using try and removing a key each time the exception is caught.
Less manual maintenance as versions change is a win!
I didn't pay close enough attention to the try grammar.
Fixed up the try syntax too.
I really dislike try, but it's almost mandatory in Python.
@tcely tcely marked this pull request as ready for review December 12, 2024 19:27
@tcely
Copy link
Contributor Author

tcely commented Dec 12, 2024

@meeb Is there a good reason Django is pinned at 3.2?

@meeb
Copy link
Owner

meeb commented Dec 13, 2024

3.2 is the latest version that works with the background tasks module which is basically now unmaintained. Upgrading past 3.2 requires replacing the tasks system.

@meeb
Copy link
Owner

meeb commented Dec 13, 2024

I'd much rather wait for the tasks system to be worked on and Django upgraded to 5.1 to natively support these sorts of features rather than backport this.

@tcely
Copy link
Contributor Author

tcely commented Dec 15, 2024

Upgrading past 3.2 requires replacing the tasks system.

Did you have something in mind for a replacement?

APScheduler with a Django implementation looked interesting, but I haven't used it before.

I also saw a 1.2.8 version available that appears to allow Django 4 versions.

@meeb
Copy link
Owner

meeb commented Dec 16, 2024

I was porting it to Celery at one point, however that got delayed as I started waiting for native background workers (Django DEP14) to merge into main.

@tcely
Copy link
Contributor Author

tcely commented Dec 17, 2024

Are you planning to wait for the implementation to be completed?

It looks like there is enough support for redis or sqlite task storage already.

https://github.com/RealOrangeOne/django-tasks

@meeb
Copy link
Owner

meeb commented Dec 17, 2024

Yeah I didn't particularly want to implement it against an in-flux reference implementation then have to make further changes once it was mainline.

@meeb
Copy link
Owner

meeb commented Feb 24, 2025

Is this still needed in this form after the bump to Django 5.X ?

@tcely
Copy link
Contributor Author

tcely commented Feb 24, 2025

The settings are going to be the same but the rest is probably unnecessary now.

@tcely
Copy link
Contributor Author

tcely commented Feb 24, 2025

We should both double check these settings are what Django 5.1 supports.

Once we are sure it works with them, this can be merged. It should help with locking problems too.

@meeb
Copy link
Owner

meeb commented Feb 24, 2025

Yeah those options should now be natively supported.

@tcely
Copy link
Contributor Author

tcely commented Feb 24, 2025

I concur.

It's ready to merge. 🎉

@meeb meeb merged commit 7d530c4 into meeb:main Feb 24, 2025
@tcely tcely deleted the sqlite-connections branch February 24, 2025 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants