`SqliteDosStorage`: Make the migrator compatible with SQLite #6429

sphuber · 2024-05-28T21:04:56Z

The majority of the SqliteDosStorage piggy-backs off of the PsqlDosStorage plugin. It also uses the PsqlDosMigrator as-is to perform the database migrations. This is not safe however, as PostgreSQL and SQLite do not have exactly the same syntax.

An example is the main_0002 revision which was added to drop the hashes of certain nodes. This uses the #- operator which is JSONB specific syntax of PostgreSQL and is not supported by SQLite. Since this migration was added before the SqliteDosStorage plugin was added, this has never caused a problems as all profiles would be new, would not have any nodes and therefore the SQL code of the migration would not actually be executed.

In preparation for any future migrations that may need to be added, the SqliteDosStorage now uses the SqliteDosMigrator. This subclasses the PsqlDosMigrator as it can still use most of the functionality, but it changes a few critical things. Most notably the location of the schema versions which now are kept individually and are no longer lent from the core.psql_dos plugin. The exception is the original schema which is identical and so is symlinked.

codecov · 2024-05-29T20:12:30Z

Codecov Report

Attention: Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.79%. Comparing base (ef60b66) to head (67603b6).
Report is 115 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/storage/sqlite_dos/backend.py	80.44%	9 Missing ⚠️
src/aiida/storage/sqlite_zip/migrations/env.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6429      +/-   ##
==========================================
+ Coverage   77.51%   77.79%   +0.29%     
==========================================
  Files         560      562       +2     
  Lines       41444    41858     +414     
==========================================
+ Hits        32120    32561     +441     
+ Misses       9324     9297      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhollas · 2024-06-05T09:11:41Z

@sphuber I punted on reviewing this since afaik it's not a blocker for 2.6.0 release. LMK I case it became a priority, otherwise I'll get to it after 20th June, unless someone else reviews first.

sphuber · 2024-06-05T09:43:44Z

Thanks @danielhollas . It is not critical for the release. Enjoy your holidays!

sphuber · 2024-07-03T06:43:51Z

@danielhollas @mbercx now that v2.6 is released, this is ready for review

danielhollas

I am definitely not the best person to review this as most of it flies over my head and I currently don't have time to delve into to storage implementation more. I mostly have questions. :-)

danielhollas · 2024-07-04T17:40:24Z

src/aiida/storage/sqlite_dos/backend.py

    """Storage implementation using Sqlite database and disk-objectstore container.

    This storage backend is not recommended for use in production. The sqlite database is not the most performant and it


I am confused, it looks like this docstring should not be here.

Indeed, this belongs under the SqliteDosStorage class which already has an updated docstring. So will just remove this.

danielhollas · 2024-07-04T17:43:12Z

src/aiida/storage/sqlite_dos/backend.py

 class SqliteDosMigrator(PsqlDosMigrator):
+    """Class for validating and migrating `sqlite_dos` storage instances.
+
+    .. important:: This class should only be accessed via the storage backend class (apart from for test purposes)


It would be nice to somewhere include a high-level description of how is the SqliteDosMigrator different from its base implementation (based off PsqlDosMigrator) i.e. what you have written up on the PR.

It is not so much that the mechanism is different, it is just that it provides a separate set of actual migrations, which will have to be sqlite or psql specific since they can use direct SQL features that are only supported by one or the other. Before, all plugins used the same migrations which were written for PSQL and so failed for sqlite databases.

Only other difference is that the new sqlite-migrator does not have the complicated additional logic to deal with migrating from legacy Django databases as this was introduced after Django had already been dropped and there were just sqlalchemy databases. I will add something to the docstring.

danielhollas · 2024-07-04T17:50:57Z

tests/cmdline/commands/test_status.py

@@ -83,6 +84,7 @@ def storage_cls(*args, **kwargs):
    assert result.exit_code is ExitCode.CRITICAL


+@pytest.mark.requires_psql


Why is this mark required here now?

Because it is explicitly using the PsqlDosMigrator, so that will fail if the test profile is run with a core.sqlite_dos profile, i.e. with the presto marker

danielhollas · 2024-07-04T17:53:06Z

src/aiida/storage/sqlite_dos/migrations/env.py

+
+    The connection should have been passed to the config, which we use to configue the migration context.
+    """
+    from aiida.storage.sqlite_zip.models import SqliteBase


We really should extract SqliteBase and related out of sqlite_zip to something like sqlite_common or sqlite_base at some point.

danielhollas · 2024-07-04T17:53:47Z

src/aiida/storage/sqlite_dos/migrations/env.py

+def run_migrations_online():
+    """Run migrations in 'online' mode.
+
+    The connection should have been passed to the config, which we use to configue the migration context.


Suggested change

The connection should have been passed to the config, which we use to configue the migration context.

The connection should have been passed to the config, which we use to configure the migration context.

danielhollas · 2024-07-04T17:54:46Z

src/aiida/storage/sqlite_dos/migrations/env.py

+# For further information on the license, see the LICENSE.txt file        #
+# For further information please visit http://www.aiida.net               #
+###########################################################################
+"""Upper level SQLAlchemy migration functions."""


I have no idea what this file is about, perhaps expanding a bit this docstring would be nice for some high-level explanation.

The majority of the `SqliteDosStorage` piggy-backs off of the `PsqlDosStorage` plugin. It also uses the `PsqlDosMigrator` as-is to perform the database migrations. This is not safe however, as PostgreSQL and SQLite do not have exactly the same syntax. An example is the `main_0002` revision which was added to drop the hashes of certain nodes. This uses the `#-` operator which is JSONB specific syntax of PostgreSQL and is not supported by SQLite. Since this migration was added before the `SqliteDosStorage` plugin was added, this has never caused a problems as all profiles would be new, would not have any nodes and therefore the SQL code of the migration would not actually be executed. In preparation for any future migrations that may need to be added, the `SqliteDosStorage` now uses the `SqliteDosMigrator`. This subclasses the `PsqlDosMigrator` as it can still use most of the functionality, but it changes a few critical things. Most notably the location of the schema versions which now are kept individually and are no longer lent from the `core.psql_dos` plugin. The initial version `main_0001_initial.py` is taken from the migration `main_0000_initial.py` of the `core.sqlite_zip` storage plugin. The only difference is that UUID fields are declared as `String(32)` instead of `CHAR(32)`. The SQLAlchemy models that are automatically generated for SQLite from the PostgreSQL-based models actually use the latter type. See `aiida.storage.sqlite_zip.models:pg_to_sqlite`.

sphuber · 2024-07-05T07:33:00Z

I am definitely not the best person to review this as most of it flies over my head and I currently don't have time to delve into to storage implementation more. I mostly have questions. :-)

Thanks regardless for the review @danielhollas . I don't think there is anyone else on the team that is more familiar with this part. I have tested in practice by actually adding a migration for the sqlite storage and it works as expected. So I am just going to merge this. If there are problems, we will come across this when we actually ever need to add a migration in the future

…am#6429) The majority of the `SqliteDosStorage` piggy-backs off of the `PsqlDosStorage` plugin. It also uses the `PsqlDosMigrator` as-is to perform the database migrations. This is not safe however, as PostgreSQL and SQLite do not have exactly the same syntax. An example is the `main_0002` revision which was added to drop the hashes of certain nodes. This uses the `#-` operator which is JSONB specific syntax of PostgreSQL and is not supported by SQLite. Since this migration was added before the `SqliteDosStorage` plugin was added, this has never caused a problems as all profiles would be new, would not have any nodes and therefore the SQL code of the migration would not actually be executed. In preparation for any future migrations that may need to be added, the `SqliteDosStorage` now uses the `SqliteDosMigrator`. This subclasses the `PsqlDosMigrator` as it can still use most of the functionality, but it changes a few critical things. Most notably the location of the schema versions which now are kept individually and are no longer lent from the `core.psql_dos` plugin. The initial version `main_0001_initial.py` is taken from the migration `main_0000_initial.py` of the `core.sqlite_zip` storage plugin. The only difference is that UUID fields are declared as `String(32)` instead of `CHAR(32)`. The SQLAlchemy models that are automatically generated for SQLite from the PostgreSQL-based models actually use the latter type. See `aiida.storage.sqlite_zip.models:pg_to_sqlite`.

sphuber force-pushed the feature/sqlite-migrations branch from 4db4bce to 4da021a Compare May 29, 2024 19:43

sphuber force-pushed the feature/sqlite-migrations branch 3 times, most recently from 32543b0 to 4a9c6a9 Compare May 30, 2024 20:23

danielhollas mentioned this pull request May 31, 2024

Feature parity between PostgreSQL and SQLite backends #6435

Open

sphuber mentioned this pull request May 31, 2024

Migration: Drop the hashes for all nodes #6350

Closed

sphuber requested review from danielhollas and mbercx May 31, 2024 11:13

sphuber force-pushed the feature/sqlite-migrations branch 2 times, most recently from 5524a9f to c2e565a Compare June 5, 2024 07:57

sphuber force-pushed the feature/sqlite-migrations branch from c2e565a to 619a6fc Compare July 2, 2024 10:35

danielhollas requested changes Jul 4, 2024

View reviewed changes

sphuber force-pushed the feature/sqlite-migrations branch from 619a6fc to 67603b6 Compare July 5, 2024 07:17

sphuber merged commit 6196dcd into aiidateam:main Jul 5, 2024
11 checks passed

sphuber deleted the feature/sqlite-migrations branch July 5, 2024 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`SqliteDosStorage`: Make the migrator compatible with SQLite #6429

`SqliteDosStorage`: Make the migrator compatible with SQLite #6429

sphuber commented May 28, 2024

codecov bot commented May 29, 2024 •

edited

Loading

danielhollas commented Jun 5, 2024 •

edited

Loading

sphuber commented Jun 5, 2024

sphuber commented Jul 3, 2024

danielhollas left a comment

danielhollas Jul 4, 2024

sphuber Jul 5, 2024

danielhollas Jul 4, 2024

sphuber Jul 5, 2024

danielhollas Jul 4, 2024

sphuber Jul 5, 2024

danielhollas Jul 4, 2024

danielhollas Jul 4, 2024

danielhollas Jul 4, 2024

sphuber commented Jul 5, 2024

		"""Storage implementation using Sqlite database and disk-objectstore container.

		This storage backend is not recommended for use in production. The sqlite database is not the most performant and it

		@@ -83,6 +84,7 @@ def storage_cls(args, *kwargs):
		assert result.exit_code is ExitCode.CRITICAL


		@pytest.mark.requires_psql

	The connection should have been passed to the config, which we use to configue the migration context.
	The connection should have been passed to the config, which we use to configure the migration context.

SqliteDosStorage: Make the migrator compatible with SQLite #6429

SqliteDosStorage: Make the migrator compatible with SQLite #6429

Conversation

sphuber commented May 28, 2024

codecov bot commented May 29, 2024 • edited Loading

Codecov Report

danielhollas commented Jun 5, 2024 • edited Loading

sphuber commented Jun 5, 2024

sphuber commented Jul 3, 2024

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Jul 5, 2024

`SqliteDosStorage`: Make the migrator compatible with SQLite #6429

`SqliteDosStorage`: Make the migrator compatible with SQLite #6429

codecov bot commented May 29, 2024 •

edited

Loading

danielhollas commented Jun 5, 2024 •

edited

Loading