Fix bug in SqlAlchemy migrations #4602
Conversation
sphuber
force-pushed
the
fix/4590/sqla-migrations
branch
from
028847c to
f0eb213
Compare
Codecov Report
@@ Coverage Diff @@
## release/1.5.1 #4602 +/- ##
=================================================
+ Coverage 79.49% 79.50% +0.02%
=================================================
Files 482 482
Lines 35329 35315 -14
=================================================
- Hits 28080 28074 -6
+ Misses 7249 7241 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thinking about this, I think we need to release this as |
sphuber
force-pushed
the
fix/4590/sqla-migrations
branch
from
f0eb213 to
c585917
Compare
There was a problem hiding this comment.
This seems all very good to me - even if of course something might always slip, alembic is a big beast :-D
Anyway, I think there might be a simple explanation for the other issue we were seeing - I think PSQL has to build a table of constraints when you put them, and to avoid blocking it will do it "in the background" while you continue operation.
Of course, if you have to use that contraint check, it will fail with the error you see.
There are ways to tell PSQL "Please create all pending constraint tables and then continue", and one could put this between the transactions, but probably this is a better approach.
The code in `aiida.backends.sqlalchemy.manager` is used, among other things, to control the database migrations. It can be used to get info like the current revision of the database, current revision of the code and to actually migrate the database. The migrations are performed through `alembic`. This library has the somewhat weird approach of loading the `env.py` file, which then actually calls the migrations. This file somehow needs to get the configuration of the environment, for example the connection to the database, and it does so by loading it from the config that is imported. This config is actually built dynamically in the `SqlBackendManager`. The old implementation was correctly building up the configuration object but was then calling the CLI commands of `alembic` to perform the various operations, which was unnecessarily complex and inefficient. For most operations, essentially everything but performing the actual migrations, one can construct a `MigrationContext` directly, which allows to inspect the state of the database, for example to get the current database revision. For anything related to the revisions in the code, e.g., to get the current head of the code, we can instantiate an instance of the `ScriptDirectory` which allows one to inspect the available revisions. Through these changes, the `env.py` file is no longer loaded everytime, even when it is not necessary, such as when simply retrieving the current revision of the database.
…5a30f0` This migration added the JSONB column `extras` to the `DbGroup` model. Since this needs to be a non-nullable column, but existing group rows would have a null value, the migration was implemented by first adding the column as nullable, after which all rows had their value set to the default, before making the column non-nullable. This approach was working when the revision was run by itself in an isolated way, which is how the unit test is run, and so that passed. However, as soon as it was run with multiple revisions in a sequence the migration would fail with the message that the `db_dbgroup` table has pending event triggers as soon as the final instruction to change the nullability of the column was executed. The cause for this was never found. As an alternative, the migration is adapted to instead create the column directly as non-nullable, but also provide a server default. This will also avoid the exception of existing rows with a null value, just as the original approach, but now when we remove the server default in a second operation, we no longer hit the problem of the pending trigger events.
sphuber
force-pushed
the
fix/4590/sqla-migrations
branch
from
c585917 to
40a8e99
Compare
Fixes #4590
SqlAlchemy: improve the alembic migration code
The code in
aiida.backends.sqlalchemy.manageris used, among otherthings, to control the database migrations. It can be used to get info
like the current revision of the database, current revision of the code
and to actually migrate the database. The migrations are performed
through
alembic. This library has the somewhat weird approach ofloading the
env.pyfile, which then actually calls the migrations.This file somehow needs to get the configuration of the environment, for
example the connection to the database, and it does so by loading it
from the config that is imported. This config is actually built
dynamically in the
SqlBackendManager.The old implementation was correctly building up the configuration
object but was then calling the CLI commands of
alembicto perform thevarious operations, which was unnecessarily complex and inefficient. For
most operations, essentially everything but performing the actual
migrations, one can construct a
MigrationContextdirectly, whichallows to inspect the state of the database, for example to get the
current database revision. For anything related to the revisions in the
code, e.g., to get the current head of the code, we can instantiate an
instance of the
ScriptDirectorywhich allows one to inspect theavailable revisions.
Through these changes, the
env.pyfile is no longer loaded everytime,even when it is not necessary, such as when simply retrieving the
current revision of the database.
SqlAlchemy: fix bug in
Groupextras migration with revision `0edcddc585917This migration added the JSONB column
extrasto theDbGroupmodel.Since this needs to be a non-nullable column, but existing group rows
would have a null value, the migration was implemented by first adding
the column as nullable, after which all rows had their value set to the
default, before making the column non-nullable.
This approach was working when the revision was run by itself in an
isolated way, which is how the unit test is run, and so that passed.
However, as soon as it was run with multiple revisions in a sequence the
migration would fail with the message that the
db_dbgrouptable haspending event triggers as soon as the final instruction to change the
nullability of the column was executed. The cause for this was never
found.
As an alternative, the migration is adapted to instead create the column
directly as non-nullable, but also provide a server default. This will
also avoid the exception of existing rows with a null value, just as the
original approach, but now when we remove the server default in a second
operation, we no longer hit the problem of the pending trigger events.