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

[Error] select_for_update cannot be used outside of a transaction. #434

Closed
edthrn opened this issue Apr 29, 2020 · 20 comments
Closed

[Error] select_for_update cannot be used outside of a transaction. #434

edthrn opened this issue Apr 29, 2020 · 20 comments

Comments

@edthrn
Copy link
Contributor

edthrn commented Apr 29, 2020

This Django error (raised from SQL Compiler) pops up in my logs and prevent any scheduled tasks to run. The error is raised from this django_q line, which is very strange since the whole try block is within the transaction.atomic() context manager.

Any idea on why this is happening and how to fix it? Thanks!

Config:

  • db: Postgres 11 with psycopg2 interface
  • django-q 1.2.1
  • django 3.0
  • python 3.8

Edit

The error is reproduced in this basic demo app

@maerteijn
Copy link
Contributor

maerteijn commented Apr 29, 2020

Can you post the DATABASES section from your django settings (leaving out credentials)?

@edthrn
Copy link
Contributor Author

edthrn commented Apr 29, 2020

Sure!

It is a really straightforward setup, as I'm in develppment mode (with dabatases running locally).

Also, note that my db router is configured to route any action comming from the django_q app label to the admin database.

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'USER': os.environ.get('DEFAULT_DB_USER', 'postgres'),
        'HOST': os.environ.get('DEFAULT_DB_HOST', '127.0.0.1'),
        'NAME': os.environ.get('DEFAULT_DB_NAME', 'default'),
        'PORT': os.environ.get('DEFAULT_DB_PORT', 5432),
    },
    'admin': {
        'ENGINE': 'django.db.backends.postgresql',
        'USER': os.environ.get('ADMIN_DB_USER', 'postgres'),
        'HOST': os.environ.get('ADMIN_DB_HOST', '127.0.0.1'),
        'NAME': os.environ.get('ADMIN_DB_NAME', 'admin'),
        'PORT': os.environ.get('ADMIN_DB_PORT', 5432),
    },
    'debug': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': os.path.join(BASE_DIR, 'debug_db.sqlite3'),
    }
}

Edit: this is my cluster setup. I added the workers param to check if the problem was arising from a multi-workers environment on my 8-core machine, but it is not: the problem stays the same with 1, 2 or 8 workers.

Q_CLUSTER = {
    'sync': False, 
    'workers': 1
}

@maerteijn
Copy link
Contributor

Could you try to use only the default connection and see what happens? I suspect that the default and admin connections are used simultaneously somehow which causes the issue.

@edthrn
Copy link
Contributor Author

edthrn commented Apr 29, 2020

Using the default connection seems to get rid of the problem indeed. I don't really understand how nor why though...

@edthrn
Copy link
Contributor Author

edthrn commented Apr 29, 2020

After a bit more tinkering, I confirm that if I route the database actions coming from django_q to the default database, the problem disappears. Thanks a lot for the hint @maerteijn

However, I'd still like to keep my default database clean of any non-business data. What would be the solution then?

Up until now, I have been routing 3rd-party apps database actions to the debug or admin connection without any issue... 🤔

@maerteijn
Copy link
Contributor

Do you run manage.py qcluster with the correct Django settings file? (eg, with the database router included?)

@edthrn
Copy link
Contributor Author

edthrn commented Apr 29, 2020

Yes absolutely, my config is made such as the main server and the qcluster always share the same DJANGO_SETTINGS_MODULE env var

@maerteijn
Copy link
Contributor

Did you configure the ORM settings to use the correct database connection (in your case: admin)?

Q_CLUSTER = {
    'name': 'DjangORM',
    'workers': 4,
    ...
    'orm': 'default'  <---
}

@edthrn
Copy link
Contributor Author

edthrn commented Apr 30, 2020

I did not, but since I use my Redis cache backend as the broker, I thought this settings was not relevant for me... Do you confirm?

This is the end of my settings files (sorry I forgot to add that line when I reported the Q config)

Q_CLUSTER['django_redis'] = 'default'

Also, I have had no issue with regular tasks, only with tasks fired by Schedules.

@maerteijn
Copy link
Contributor

I did not, but since I use my Redis cache backend as the broker, I thought this settings was not relevant for me... Do you confirm?

I confirm, I assumed the ORM backend as you did not mention you used the django_redis backend.

So something there isn't going well when using multiple database backends for some reason. Can you post your database router as well? (I'm not able to put much time in this but it can certainly help for others as well)

@edthrn
Copy link
Contributor Author

edthrn commented Apr 30, 2020

Thanks for your help, much appreciated.

My DATABASE_ROUTERS setting is a single-item list, pointing to this class:

# I would add `django_q` in this list
ADMIN_APPS = ['admin', 'auth', 'contenttypes', 'sessions']  

class DatabaseRouter:

    @staticmethod
    def is_admin(model):
        return model._meta.app_label in ADMIN_APPS

    def db_for_read(self, model, **hints):
        if self.is_admin(model):
            return 'admin'
        return 'default'

    def db_for_write(self, model, **hints):
        if self.is_admin(model):
            return 'admin'
        return 'default'

    def allow_migrate(self, db, app_label, model_name='', **hints):
        if app_label in ADMIN_APPS:
            return db == 'admin'
        return db == 'default'

@edthrn
Copy link
Contributor Author

edthrn commented May 1, 2020

After a little bit more digging, I found out that the error does not show up with a basic SQLite config. I did not take the time to check with other db backends though

How to reproduce the error

To help people who'd want to help me find out what the hell is happening, I have setup a very basic demo project, check out the Github repo.

There is the very minimum required to make it fail as it happened to me.

@maerteijn
Copy link
Contributor

Sqlite does not do select_for_update locks so that’s why. I’ll check your repo in the following days to see if I can find what’s going on.

@maerteijn
Copy link
Contributor

So I guess what's happening here is that the atomic transaction is started on the wrong database connection. You can specify the database with the using option. I'll investigate a bit further.

@maerteijn
Copy link
Contributor

So it looks like two things are causing problems here when using a database router:

In my test setup (which is your test repo) it looks like these changes fix it: https://github.com/Koed00/django-q/compare/master...maerteijn:fix-database-router?expand=1

Could you test the fix-database-router branch to see if you can confirm these fixes solve your issue?

@edthrn
Copy link
Contributor Author

edthrn commented May 1, 2020

I ran a quick test with your patch, and I confirm that it does fix the issue, both in the demo app and my real app 💯

@maerteijn
Copy link
Contributor

That's cool! I'll review once again and check for the implications of these changes and will add a PR when ready. Have a nice weekend!

@edthrn
Copy link
Contributor Author

edthrn commented May 13, 2020

Hello @maerteijn

Just in case you were under heavy load these days, do you want me to submit a PR?

@maerteijn
Copy link
Contributor

I will do it this Friday, I didn’t forget 🙂

@edthrn
Copy link
Contributor Author

edthrn commented Jun 12, 2020

Fixed in release 1.2.2, tested and approved!

Thanks @maerteijn and @Koed00 💯

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

No branches or pull requests

2 participants