[rel-db] Initial migration #3152
Conversation
…/openslides-backend into RelDB-Migration
…s-backend into RelDB-Migration
write migration index readd old schema for migration
* remove 'progress' and 'clear-collectionfield-tables'
* reenable migrations in entrypoint * remove writable marker * extend reset command a bit * update legacy example json
Co-authored-by: Alexej <33332102+4echow@users.noreply.github.com>
luisa-beerboom
left a comment
luisa-beerboom
left a comment
There was a problem hiding this comment.
Only looked at some of it so far
| "SELECT schemaname, tablename from pg_tables where schemaname in ('public', 'vote');" | ||
| ).fetchall() | ||
| return [ | ||
| f"{row.get('schemaname', '')}.{row.get('tablename', '')}" for row in rows if row not in OLD_TABLES # type: ignore |
| - You can write migrations in the backend and also adjust the Datastore at the same time since both are mounted into the container. | ||
|
|
||
| The following scripts can be used to make snapshots and trying out new migrations | ||
| The migrations itself are in the `migrations` folder. Each file must start with a four digit number and can include certain functions that can be found by the loader. A psycopg cursor object will be passed as function parameter. |
There was a problem hiding this comment.
| The migrations itself are in the `migrations` folder. Each file must start with a four digit number and can include certain functions that can be found by the loader. A psycopg cursor object will be passed as function parameter. | |
| The migrations themselves are in the `migrations` folder. Each file must start with a four digit number and can include certain functions that can be found by the loader. A psycopg cursor object will be passed as function parameter. |
|
|
||
| def setup_migration_relations(self) -> None: | ||
| """Sets the tables and views used within the migration and copies their data.""" | ||
| # TODO this method needs to be tested. |
There was a problem hiding this comment.
Is it even possible to test this before the first post-relDB Migration happens? If it isn't, perhaps write an issue?
| Returns: | ||
| - None |
There was a problem hiding this comment.
Is this part of the description necessary if you already have the type annotation on the function?
| if getattr(migration_module, "IN_MEMORY", False): | ||
| # TODO In-Memory migration | ||
| migration_module.in_memory_method() | ||
| else: |
There was a problem hiding this comment.
IMM is not planned after relDB update. The TODO etc can be removed.
| @staticmethod | ||
| def load_migrations() -> None: | ||
| """ | ||
| Checks wether current migration_index is equal to or above the FIRST_REL_DB_MIGRATION and |
There was a problem hiding this comment.
| Checks wether current migration_index is equal to or above the FIRST_REL_DB_MIGRATION and | |
| Checks whether current migration_index is equal to or above the FIRST_REL_DB_MIGRATION and |
*whether
| if tmp := curs.execute("SELECT migration_index FROM version;").fetchall(): | ||
| return [elem.get("migration_index", 0) for elem in tmp] | ||
| raise MigrationException( | ||
| "No migration index could not be acquired from database." |
There was a problem hiding this comment.
Double negation in error message :(
| ).fetchone() | ||
| assert ( | ||
| version_table_exists | ||
| ), "Something really blew up checking the version tables existence." |
| verbose: bool = False, | ||
| print_fn: PrintFunction = print, | ||
| ) -> None: | ||
| """init""" |
There was a problem hiding this comment.
Description very descriptive. Delete.
| if response: | ||
| return response.get("count", 0) | ||
| else: | ||
| return 0 |
There was a problem hiding this comment.
| if response: | |
| return response.get("count", 0) | |
| else: | |
| return 0 | |
| return (response or {}).get("count", 0) |
?
| if mi > current_migration_index | ||
| if state_per_mi[mi] | ||
| in (MigrationState.MIGRATION_REQUIRED, MigrationState.MIGRATION_RUNNING) |
luisa-beerboom
left a comment
luisa-beerboom
left a comment
There was a problem hiding this comment.
Looked at everything that is there so far
| if result is not None: | ||
| if len(result) > 0: |
There was a problem hiding this comment.
no need for two lines, connect the conditions with and
There was a problem hiding this comment.
Could also just use a questionmark conditional on the return statement to combine all of ln 59 - 62
| # 1.1) OrganizationFields are always generated on DB side | ||
| if isinstance(field_class, OrganizationField): | ||
| return True | ||
|
|
||
| # 1.2) ViewFields are solely for the DB table view | ||
| elif field_class.is_view_field: | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
| # 1.1) OrganizationFields are always generated on DB side | |
| if isinstance(field_class, OrganizationField): | |
| return True | |
| # 1.2) ViewFields are solely for the DB table view | |
| elif field_class.is_view_field: | |
| return True | |
| return False | |
| return isinstance(field_class, OrganizationField) or field_class.is_view_field |
| result = cur.execute( | ||
| "SELECT theme_id FROM organization_t WHERE id=1;" | ||
| ).fetchone() | ||
| assert result is not None, "1:1 relation in organization not filled." |
There was a problem hiding this comment.
This part and below:
Sometimes you're writing result = cur.execute("...").fetchone() and then assert result = ..., and sometimes you're writing cur.execute("...") and then cur.fetchone().
Two questions:
- Why not more consistent?
- Why not make a helper function that hides the repeating parts of this pattern for the sake of readability?
| for idx, row in enumerate(curs.fetchall(), 99): | ||
| if row["migration_index"] == LAST_NON_REL_MIGRATION: | ||
| continue | ||
| assert { | ||
| "migration_index": idx, | ||
| "migration_state": MigrationState.FINALIZATION_REQUIRED, | ||
| } == row |
There was a problem hiding this comment.
Why start the enumeration with 99 and why skip the last non reldb migration index?
What are you checking for here?
| if row["migration_index"] == LAST_NON_REL_MIGRATION: | ||
| continue | ||
| assert { | ||
| "migration_index": idx, | ||
| "migration_state": MigrationState.FINALIZED, | ||
| } == row |
There was a problem hiding this comment.
Same here why not check it for the last non RelDB migration?
| Input: | ||
| n/a | ||
| Returns: | ||
| n/a |
There was a problem hiding this comment.
I don't think these 4 lines are necessary. Not for the other test cases either.
| The test cases initially may seem to be spartanic caused by the lack of testing of integrity | ||
| after transfering the data from the key-value-store into their respective tables. | ||
| What is tested is the correct creation of intermediate tables and simple relations exemplary as we | ||
| can trust that it works for other tables if it worked for one. | ||
| Also it is tested that the new tables are created on top of an old basis and the old tables are deleted. | ||
| It should be only used like this in this migration test since it leads to a performance problem | ||
| once the actual connection context is entered the first time after the database was dropped and recreated. |
There was a problem hiding this comment.
This comment seems to be under every single test case. Isn't there a more central place this could be put? The check_data function or the top of the file maybe?
Also the text seems to suggest that the integrity of the complete instance is not checked. That breaks with our previous MO as not everything that is changed is also checked. Thinking in terms of what this would mean in the old system: It would risk missing bugs in something as critical as this migration and getting corrupt data as a result. Or missing data. Idk how well that philosophy translates into the new system, but if the risk exists, considering how important this is, I'd suggest testing the content of the new tables more thoroughly in at least one test.
There was a problem hiding this comment.
In the old system, as far as I recall, the database check was always called automatically after every finalize call in in the test of whatever migration was currently the most recent.
5 participants
Continuation of #2520