-
Notifications
You must be signed in to change notification settings - Fork 218
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
WIP: Check if table already exists before creating it #158
base: master
Are you sure you want to change the base?
Conversation
If we have SQLALCHEMYBACKEND_DROP_ALL_TABLES = False, then without this fix we will fail to start db and strategy workers with an existing database.
This check is not enough to prevent errors during concurrent table creation, at least with postgres. Although this looks more like a sqlalchemy bug:
|
@lopuhin concurrent table creation is something we should avoid doing. Such a behavior isn't expected by both DB servers and clients. I recommend to redesign your application to avoid doing this. Concurrent truncation of the table should work fine, btw. |
@@ -132,7 +132,7 @@ def strategy_worker(cls, manager): | |||
if drop_all_tables: | |||
if model.__table__.name in inspector.get_table_names(): | |||
model.__table__.drop(bind=b.engine) | |||
model.__table__.create(bind=b.engine) | |||
model.__table__.create(bind=b.engine, checkfirst=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is happening because SQLA has non-unified behavior between various engines. For example that code works well for sqlite, without need of checkfirst=True
. However, I still think it makes sense to merge it.
@sibiryakov thanks, I'll try to avoid concurrent table creation, that makes sense! |
I would like to test that behavior. What if we'll be testing our backends with |
Yep, I'll add the tests (found them now). |
If we have SQLALCHEMYBACKEND_DROP_ALL_TABLES = False, then without this fix we will fail to start db and strategy workers with an existing database, because the tables are already there, but we try to create them.
Potentially, creating the table without checking for existance could cause troubles even with SQLALCHEMYBACKEND_DROP_ALL_TABLES = True, when several workers would try to create required tables concurrently, but I never tried to reproduce this.