Message ID | 20210716171940.3827847-3-dja@axtens.net |
---|---|
State | New |
Headers | show |
Series | RFC: Patchwork 2.2->3.0 migration improvements | expand |
On Jul 16, 2021, at 10:20 AM, Daniel Axtens <dja@axtens.net> wrote: > > - we do a lot of legwork to prevent there being a column with the same > name in a base class and subclass. This is purely for django's benefit > as databases don't see the class relationship and handle column names > being the same across tables just fine. Unfortunately renaming columns > in MySQL/MariaDB is _extremely_ expensive. So just don't do it: lie to > django about the names of the duplicate columns. I believe that modern versions can ALTER TABLE ONLINE to do a column rename without having to copy all the data. Should also work for adding columns (some types). Arguably Django could probably switch to trying specifying ONLINE as the method and falling back to offline (full copy) if it can’t do it online.
Stewart Smith <stewart@flamingspork.com> writes: > On Jul 16, 2021, at 10:20 AM, Daniel Axtens <dja@axtens.net> wrote: >> >> - we do a lot of legwork to prevent there being a column with the same >> name in a base class and subclass. This is purely for django's benefit >> as databases don't see the class relationship and handle column names >> being the same across tables just fine. Unfortunately renaming columns >> in MySQL/MariaDB is _extremely_ expensive. So just don't do it: lie to >> django about the names of the duplicate columns. > > I believe that modern versions can ALTER TABLE ONLINE to do a column rename without having to copy all the data. Should also work for adding columns (some types). > > Arguably Django could probably switch to trying specifying ONLINE as the method and falling back to offline (full copy) if it can’t do it online. Oh that's handy to know, I'll see if I can use that to simplify some of the series. Arguably Django should use it but I'm definitely not going to try to monkey-patch that in! Kind regards, Daniel
On Sat, 2021-07-17 at 03:19 +1000, Daniel Axtens wrote: > - we do a lot of legwork to prevent there being a column with the same > name in a base class and subclass. This is purely for django's benefit > as databases don't see the class relationship and handle column names > being the same across tables just fine. Unfortunately renaming columns > in MySQL/MariaDB is _extremely_ expensive. So just don't do it: lie to > django about the names of the duplicate columns. > > - This means we don't have to worry about deleting the index and the > unique_together constraints. Let deleting the table do all of that at > once. > > - I don't think we gain anything from deleting foreign keys out of the > patch table before deleting it. I initially thought this might relate > to an ON DELETE CASCADE, but the ON DELETE rules in the patches table > relate to what happens to the row in the patches tables if you delete > an element from the associated table, not the other way around. > > Signed-off-by: Daniel Axtens <dja@axtens.net> This seems very sensible and I see no reason not to apply it. I've held off though because your reply to Stewart down thread suggests you might want to rework this? If not, feel free to apply this (add a 'Related: #420' trailer to the commit message) and backport ('git cherry-pick -x') back to stable/3.0, or let me know and I'll do it. Reviewed-by: Stephen Finucane <stephen@that.guru> Thanks! Stephen > --- > .../migrations/0043_merge_patch_submission.py | 140 +++++++++--------- > 1 file changed, 68 insertions(+), 72 deletions(-) > > diff --git a/patchwork/migrations/0043_merge_patch_submission.py b/patchwork/migrations/0043_merge_patch_submission.py > index d351892ef7f2..465e527812ba 100644 > --- a/patchwork/migrations/0043_merge_patch_submission.py > +++ b/patchwork/migrations/0043_merge_patch_submission.py > @@ -10,16 +10,16 @@ def migrate_data(apps, schema_editor): > schema_editor.execute( > """ > UPDATE patchwork_submission > - SET archived = patchwork_patch.archived2, > - commit_ref = patchwork_patch.commit_ref2, > - delegate_id = patchwork_patch.delegate2_id, > - diff = patchwork_patch.diff2, > - hash = patchwork_patch.hash2, > - number = patchwork_patch.number2, > - pull_url = patchwork_patch.pull_url2, > - related_id = patchwork_patch.related2_id, > - series_id = patchwork_patch.series2_id, > - state_id = patchwork_patch.state2_id > + SET archived = patchwork_patch.archived, > + commit_ref = patchwork_patch.commit_ref, > + delegate_id = patchwork_patch.delegate_id, > + diff = patchwork_patch.diff, > + hash = patchwork_patch.hash, > + number = patchwork_patch.number, > + pull_url = patchwork_patch.pull_url, > + related_id = patchwork_patch.related_id, > + series_id = patchwork_patch.series_id, > + state_id = patchwork_patch.state_id > FROM patchwork_patch > WHERE patchwork_submission.id = patchwork_patch.submission_ptr_id > """ > @@ -28,16 +28,16 @@ def migrate_data(apps, schema_editor): > schema_editor.execute( > """ > UPDATE patchwork_submission, patchwork_patch > - SET patchwork_submission.archived = patchwork_patch.archived2, > - patchwork_submission.commit_ref = patchwork_patch.commit_ref2, > - patchwork_submission.delegate_id = patchwork_patch.delegate2_id, > - patchwork_submission.diff = patchwork_patch.diff2, > - patchwork_submission.hash = patchwork_patch.hash2, > - patchwork_submission.number = patchwork_patch.number2, > - patchwork_submission.pull_url = patchwork_patch.pull_url2, > - patchwork_submission.related_id = patchwork_patch.related2_id, > - patchwork_submission.series_id = patchwork_patch.series2_id, > - patchwork_submission.state_id = patchwork_patch.state2_id > + SET patchwork_submission.archived = patchwork_patch.archived, > + patchwork_submission.commit_ref = patchwork_patch.commit_ref, > + patchwork_submission.delegate_id = patchwork_patch.delegate_id, > + patchwork_submission.diff = patchwork_patch.diff, > + patchwork_submission.hash = patchwork_patch.hash, > + patchwork_submission.number = patchwork_patch.number, > + patchwork_submission.pull_url = patchwork_patch.pull_url, > + patchwork_submission.related_id = patchwork_patch.related_id, > + patchwork_submission.series_id = patchwork_patch.series_id, > + patchwork_submission.state_id = patchwork_patch.state_id > WHERE patchwork_submission.id = patchwork_patch.submission_ptr_id > """ # noqa > ) > @@ -50,16 +50,16 @@ def migrate_data(apps, schema_editor): > pull_url, related_id, series_id, state_id > ) = ( > SELECT > - patchwork_patch.archived2, > - patchwork_patch.commit_ref2, > - patchwork_patch.delegate2_id, > - patchwork_patch.diff2, > - patchwork_patch.hash2, > - patchwork_patch.number2, > - patchwork_patch.pull_url2, > - patchwork_patch.related2_id, > - patchwork_patch.series2_id, > - patchwork_patch.state2_id > + patchwork_patch.archived, > + patchwork_patch.commit_ref, > + patchwork_patch.delegate_id, > + patchwork_patch.diff, > + patchwork_patch.hash, > + patchwork_patch.number, > + patchwork_patch.pull_url, > + patchwork_patch.related_id, > + patchwork_patch.series_id, > + patchwork_patch.state_id > FROM patchwork_patch > WHERE patchwork_patch.submission_ptr_id = patchwork_submission.id > ) > @@ -152,39 +152,44 @@ class Migration(migrations.Migration): > # rename all the fields on 'Patch' so we don't have duplicates when we > # add them to 'Submission' > > - migrations.RemoveIndex( > - model_name='patch', name='patch_list_covering_idx', > - ), > - migrations.AlterUniqueTogether(name='patch', unique_together=set([]),), > - migrations.RenameField( > - model_name='patch', old_name='archived', new_name='archived2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='commit_ref', new_name='commit_ref2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='delegate', new_name='delegate2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='diff', new_name='diff2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='hash', new_name='hash2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='number', new_name='number2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='pull_url', new_name='pull_url2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='related', new_name='related2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='series', new_name='series2', > - ), > - migrations.RenameField( > - model_name='patch', old_name='state', new_name='state2', > + # Again we only to rename these fields in the django view of the world (state) > + # to avoid errors about the same fields being in the base class (Submission) > + # and a subclass (Patch). The db doesn't care, and we migrate the data in raw > + # SQL, so basically just lie to django here. > + migrations.SeparateDatabaseAndState( > + database_operations=[], > + state_operations=[ > + migrations.RenameField( > + model_name='patch', old_name='archived', new_name='archived2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='commit_ref', new_name='commit_ref2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='delegate', new_name='delegate2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='diff', new_name='diff2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='hash', new_name='hash2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='number', new_name='number2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='pull_url', new_name='pull_url2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='related', new_name='related2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='series', new_name='series2', > + ), > + migrations.RenameField( > + model_name='patch', old_name='state', new_name='state2', > + ), > + ], > ), > > # add the fields found on 'Patch' to 'Submission' > @@ -306,15 +311,6 @@ class Migration(migrations.Migration): > ), > ), > > - # remove the foreign key fields from the 'Patch' model > - > - migrations.RemoveField(model_name='patch', name='delegate2',), > - migrations.RemoveField(model_name='patch', name='patch_project',), > - migrations.RemoveField(model_name='patch', name='related2',), > - migrations.RemoveField(model_name='patch', name='series2',), > - migrations.RemoveField(model_name='patch', name='state2',), > - migrations.RemoveField(model_name='patch', name='submission_ptr',), > - > # drop the 'Patch' model and rename 'Submission' to 'Patch' > > migrations.DeleteModel(name='Patch',),
diff --git a/patchwork/migrations/0043_merge_patch_submission.py b/patchwork/migrations/0043_merge_patch_submission.py index d351892ef7f2..465e527812ba 100644 --- a/patchwork/migrations/0043_merge_patch_submission.py +++ b/patchwork/migrations/0043_merge_patch_submission.py @@ -10,16 +10,16 @@ def migrate_data(apps, schema_editor): schema_editor.execute( """ UPDATE patchwork_submission - SET archived = patchwork_patch.archived2, - commit_ref = patchwork_patch.commit_ref2, - delegate_id = patchwork_patch.delegate2_id, - diff = patchwork_patch.diff2, - hash = patchwork_patch.hash2, - number = patchwork_patch.number2, - pull_url = patchwork_patch.pull_url2, - related_id = patchwork_patch.related2_id, - series_id = patchwork_patch.series2_id, - state_id = patchwork_patch.state2_id + SET archived = patchwork_patch.archived, + commit_ref = patchwork_patch.commit_ref, + delegate_id = patchwork_patch.delegate_id, + diff = patchwork_patch.diff, + hash = patchwork_patch.hash, + number = patchwork_patch.number, + pull_url = patchwork_patch.pull_url, + related_id = patchwork_patch.related_id, + series_id = patchwork_patch.series_id, + state_id = patchwork_patch.state_id FROM patchwork_patch WHERE patchwork_submission.id = patchwork_patch.submission_ptr_id """ @@ -28,16 +28,16 @@ def migrate_data(apps, schema_editor): schema_editor.execute( """ UPDATE patchwork_submission, patchwork_patch - SET patchwork_submission.archived = patchwork_patch.archived2, - patchwork_submission.commit_ref = patchwork_patch.commit_ref2, - patchwork_submission.delegate_id = patchwork_patch.delegate2_id, - patchwork_submission.diff = patchwork_patch.diff2, - patchwork_submission.hash = patchwork_patch.hash2, - patchwork_submission.number = patchwork_patch.number2, - patchwork_submission.pull_url = patchwork_patch.pull_url2, - patchwork_submission.related_id = patchwork_patch.related2_id, - patchwork_submission.series_id = patchwork_patch.series2_id, - patchwork_submission.state_id = patchwork_patch.state2_id + SET patchwork_submission.archived = patchwork_patch.archived, + patchwork_submission.commit_ref = patchwork_patch.commit_ref, + patchwork_submission.delegate_id = patchwork_patch.delegate_id, + patchwork_submission.diff = patchwork_patch.diff, + patchwork_submission.hash = patchwork_patch.hash, + patchwork_submission.number = patchwork_patch.number, + patchwork_submission.pull_url = patchwork_patch.pull_url, + patchwork_submission.related_id = patchwork_patch.related_id, + patchwork_submission.series_id = patchwork_patch.series_id, + patchwork_submission.state_id = patchwork_patch.state_id WHERE patchwork_submission.id = patchwork_patch.submission_ptr_id """ # noqa ) @@ -50,16 +50,16 @@ def migrate_data(apps, schema_editor): pull_url, related_id, series_id, state_id ) = ( SELECT - patchwork_patch.archived2, - patchwork_patch.commit_ref2, - patchwork_patch.delegate2_id, - patchwork_patch.diff2, - patchwork_patch.hash2, - patchwork_patch.number2, - patchwork_patch.pull_url2, - patchwork_patch.related2_id, - patchwork_patch.series2_id, - patchwork_patch.state2_id + patchwork_patch.archived, + patchwork_patch.commit_ref, + patchwork_patch.delegate_id, + patchwork_patch.diff, + patchwork_patch.hash, + patchwork_patch.number, + patchwork_patch.pull_url, + patchwork_patch.related_id, + patchwork_patch.series_id, + patchwork_patch.state_id FROM patchwork_patch WHERE patchwork_patch.submission_ptr_id = patchwork_submission.id ) @@ -152,39 +152,44 @@ class Migration(migrations.Migration): # rename all the fields on 'Patch' so we don't have duplicates when we # add them to 'Submission' - migrations.RemoveIndex( - model_name='patch', name='patch_list_covering_idx', - ), - migrations.AlterUniqueTogether(name='patch', unique_together=set([]),), - migrations.RenameField( - model_name='patch', old_name='archived', new_name='archived2', - ), - migrations.RenameField( - model_name='patch', old_name='commit_ref', new_name='commit_ref2', - ), - migrations.RenameField( - model_name='patch', old_name='delegate', new_name='delegate2', - ), - migrations.RenameField( - model_name='patch', old_name='diff', new_name='diff2', - ), - migrations.RenameField( - model_name='patch', old_name='hash', new_name='hash2', - ), - migrations.RenameField( - model_name='patch', old_name='number', new_name='number2', - ), - migrations.RenameField( - model_name='patch', old_name='pull_url', new_name='pull_url2', - ), - migrations.RenameField( - model_name='patch', old_name='related', new_name='related2', - ), - migrations.RenameField( - model_name='patch', old_name='series', new_name='series2', - ), - migrations.RenameField( - model_name='patch', old_name='state', new_name='state2', + # Again we only to rename these fields in the django view of the world (state) + # to avoid errors about the same fields being in the base class (Submission) + # and a subclass (Patch). The db doesn't care, and we migrate the data in raw + # SQL, so basically just lie to django here. + migrations.SeparateDatabaseAndState( + database_operations=[], + state_operations=[ + migrations.RenameField( + model_name='patch', old_name='archived', new_name='archived2', + ), + migrations.RenameField( + model_name='patch', old_name='commit_ref', new_name='commit_ref2', + ), + migrations.RenameField( + model_name='patch', old_name='delegate', new_name='delegate2', + ), + migrations.RenameField( + model_name='patch', old_name='diff', new_name='diff2', + ), + migrations.RenameField( + model_name='patch', old_name='hash', new_name='hash2', + ), + migrations.RenameField( + model_name='patch', old_name='number', new_name='number2', + ), + migrations.RenameField( + model_name='patch', old_name='pull_url', new_name='pull_url2', + ), + migrations.RenameField( + model_name='patch', old_name='related', new_name='related2', + ), + migrations.RenameField( + model_name='patch', old_name='series', new_name='series2', + ), + migrations.RenameField( + model_name='patch', old_name='state', new_name='state2', + ), + ], ), # add the fields found on 'Patch' to 'Submission' @@ -306,15 +311,6 @@ class Migration(migrations.Migration): ), ), - # remove the foreign key fields from the 'Patch' model - - migrations.RemoveField(model_name='patch', name='delegate2',), - migrations.RemoveField(model_name='patch', name='patch_project',), - migrations.RemoveField(model_name='patch', name='related2',), - migrations.RemoveField(model_name='patch', name='series2',), - migrations.RemoveField(model_name='patch', name='state2',), - migrations.RemoveField(model_name='patch', name='submission_ptr',), - # drop the 'Patch' model and rename 'Submission' to 'Patch' migrations.DeleteModel(name='Patch',),
- we do a lot of legwork to prevent there being a column with the same name in a base class and subclass. This is purely for django's benefit as databases don't see the class relationship and handle column names being the same across tables just fine. Unfortunately renaming columns in MySQL/MariaDB is _extremely_ expensive. So just don't do it: lie to django about the names of the duplicate columns. - This means we don't have to worry about deleting the index and the unique_together constraints. Let deleting the table do all of that at once. - I don't think we gain anything from deleting foreign keys out of the patch table before deleting it. I initially thought this might relate to an ON DELETE CASCADE, but the ON DELETE rules in the patches table relate to what happens to the row in the patches tables if you delete an element from the associated table, not the other way around. Signed-off-by: Daniel Axtens <dja@axtens.net> --- .../migrations/0043_merge_patch_submission.py | 140 +++++++++--------- 1 file changed, 68 insertions(+), 72 deletions(-)