Skip to content

Commit

Permalink
Migration: Change connector_geom id to bigint
Browse files Browse the repository at this point in the history
  • Loading branch information
@unidesigner
unidesigner committed Oct 28, 2019
1 parent 6ebe988 commit 65b4044
Showing 1 changed file with 21 additions and 0 deletions.
21 changes: 21 additions & 0 deletions django/applications/catmaid/migrations/0088_connector_geom_fix_bigint.py
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Generated by Django 2.2.5 on 2019-10-28 11:32

from django.db import migrations

forward = """
ALTER TABLE connector_geom ALTER COLUMN id TYPE bigint;
"""

backward = """
ALTER TABLE connector_geom ALTER COLUMN id TYPE integer;
"""

class Migration(migrations.Migration):

dependencies = [
('catmaid', '0087_add_nblast_normalization_mode'),
]

operations = [
migrations.RunSQL(forward, backward)
]

7 comments on commit 65b4044

@tomka
Copy link
Contributor

@tomka tomka commented on 65b4044 Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but this is also part of the migration in PR #1887, which I haven't merged yet. It would be good if we can merge this first, before we do additional int/bigint changes. In order to do so, I have to review it one more time though and test again. The goal is to do that in the next two weeks, because I'd like to cut a new release.

@tomka
Copy link
Contributor

@tomka tomka commented on 65b4044 Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, doing an ALTER TABLE … ALTER COLUMN … will rewrite the complete table. On large instances, especially with automatically generated connector nodes, this table will be large. Generally, space is only reclaimed in these situations if VACUUM FULL is run, which is expensive and causes yet another table rewrite. To avoid this, the migration I referenced above create a completely new table for both connector_geom and treenode_connector_edge, copies the data over (making the IDs 64 bit), updates all references and indices, and finally deleted the old table. This reclaims space immediately (i.e. no VACUUM FULL needed) and is therefore more friendly to large setups. All this happens in the referenced PR in file 0076_fix_treenode_connector_edge_foreign_key_types.py.

Therefore I feel it would be better if you could revert this change and I will go through the PR #1887 in the next two days and merge it. This isn't ideal, because your migration might be in use already, but I also don't think it will be really problematic, because the new migration will essentially do the same.

@tomka
Copy link
Contributor

@tomka tomka commented on 65b4044 Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about how to change this migration a bit more, I think the best way to go about this is the following: I will split up migration 0076_fix_treenode_connector_edge_foreign_key_types.py in #1887 into one that runs for connector_geom and one that runs for treenode_connector_edge (it handles both in one transaction). I will replace your migration code above (65b4044) with the connector_geom part of #1887 and add the treenode_connector_edge part as a separate migration. This way you don't have to update your local database (I assume you have this migration applied already), because the outcome would be the same.

Also as a general point, I believe it is is useful to mention aspects like table rewrites, space constraints, etc. in the comments/documentation of migration like this.

@unidesigner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, splitting this into two migration sounds good to me. I reverted my commit. I was not aware of the pull request #1887, and thanks for the explanation of how to make such migrations more efficient.

@tomka
Copy link
Contributor

@tomka tomka commented on 65b4044 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, thanks for the quick fix! I will merge this first part of the #1887 including the changes you wanted to make as soon as possible and try to have a look at the remaining schema changes too.

@tomka
Copy link
Contributor

@tomka tomka commented on 65b4044 Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed and moved the 64 Bit migrations dealing with connector_geom and treenode_connector_edge as well as change_request into dev now. I kep the connector_geom migration name you chose for your implementation and since the outcome of this migration is the same, you should be able to just --fake migration 0088_connector_geom_fix_bigint. Of course only this migration and only if you have your migration already applied. The remaining 64 Bit changes for the concept based tables will hopefully make it into dev soon as well.

@unidesigner
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tomka, I'll get to it next week, its looking good.

Please sign in to comment.