[2/4] migrations: 0043_merge_patch_submission: do less work

- 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(-)

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',),