| Message ID | 1458248656-3342-1-git-send-email-andy.doan@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 17 Mar 16:04, Andy Doan wrote: > By handling comment copying/deletion via SQL we can make migration take > <3 minutes rather than >30minutes for big instances. The SQL is vendor > specific, so a similar query would need to be added to support MySQL. > > Signed-off-by: Andy Doan <andy.doan@linaro.org> Nice. I'll review this (and the REST API proposal) tomorrow morning! Stephen
On 17 Mar 16:04, Andy Doan wrote: > By handling comment copying/deletion via SQL we can make migration take > <3 minutes rather than >30minutes for big instances. The SQL is vendor > specific, so a similar query would need to be added to support MySQL. > > Signed-off-by: Andy Doan <andy.doan@linaro.org> Some nits below, but nothing important so happy to merge. Any chance you want to do the same for MySQL? :) Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> Stephen > --- > .../0007_move_comment_content_to_patch_content.py | 89 +++++++++++++--------- > 1 file changed, 51 insertions(+), 38 deletions(-) > > diff --git a/patchwork/migrations/0007_move_comment_content_to_patch_content.py b/patchwork/migrations/0007_move_comment_content_to_patch_content.py > index 63d57ba..aea65fe 100644 > --- a/patchwork/migrations/0007_move_comment_content_to_patch_content.py > +++ b/patchwork/migrations/0007_move_comment_content_to_patch_content.py > @@ -1,47 +1,60 @@ > # -*- coding: utf-8 -*- > from __future__ import unicode_literals > > -from django.db import migrations > - > - > -def copy_comment_field(apps, schema_editor): > - Comment = apps.get_model('patchwork', 'Comment') > - Patch = apps.get_model('patchwork', 'Patch') > - > - for patch in Patch.objects.all(): > - try: > - # when available, this can only return one entry due to the > - # unique_together constraint > - comment = Comment.objects.get(patch=patch, msgid=patch.msgid) > - except Comment.DoesNotExist: > - # though there's no requirement to actually have a comment > - continue > - > - patch.content = comment.content > - patch.save() > +from django.db import connection, migrations > + > +if connection.vendor == 'postgresql' I'd rather this check be done in each function, rather than having multiple functions. It just seems...cleaner. > + def copy_comment_field(apps, schema_editor): > + schema_editor.execute(''' > + UPDATE patchwork_patch > + SET content = patchwork_comment.content > + FROM patchwork_comment > + WHERE patchwork_patch.id=patchwork_comment.patch_id > + AND patchwork_patch.msgid=patchwork_comment.msgid > + ''') > + > + def remove_duplicate_comments(apps, schema_editor): > + schema_editor.execute(''' > + DELETE FROM patchwork_comment > + USING patchwork_patch > + WHERE patchwork_patch.id=patchwork_comment.patch_id > + AND patchwork_patch.msgid=patchwork_comment.msgid > + ''') > +else: > + def copy_comment_field(apps, schema_editor): > + Comment = apps.get_model('patchwork', 'Comment') > + Patch = apps.get_model('patchwork', 'Patch') > + > + for patch in Patch.objects.all(): > + try: > + # when available, this can only return one entry due to the > + # unique_together constraint > + comment = Comment.objects.get(patch=patch, msgid=patch.msgid) > + except Comment.DoesNotExist: > + # though there's no requirement to actually have a comment > + continue > + > + patch.content = comment.content > + patch.save() > + > + def remove_duplicate_comments(apps, schema_editor): > + Comment = apps.get_model('patchwork', 'Comment') > + Patch = apps.get_model('patchwork', 'Patch') > + > + for patch in Patch.objects.all(): > + try: > + # when available, this can only return one entry due to the > + # unique_together constraint > + comment = Comment.objects.get(patch=patch, msgid=patch.msgid) > + comment.delete() > + except Comment.DoesNotExist: > + # though there's no requirement to actually have a comment > + continue > > > def uncopy_comment_field(apps, schema_editor): > - Patch = apps.get_model('patchwork', 'Patch') > - > - for patch in Patch.objects.all(): > - patch.content = None > - patch.save() > - > - > -def remove_duplicate_comments(apps, schema_editor): > - Comment = apps.get_model('patchwork', 'Comment') > - Patch = apps.get_model('patchwork', 'Patch') > - > - for patch in Patch.objects.all(): > - try: > - # when available, this can only return one entry due to the > - # unique_together constraint > - comment = Comment.objects.get(patch=patch, msgid=patch.msgid) > - comment.delete() > - except Comment.DoesNotExist: > - # though there's no requirement to actually have a comment > - continue > + # This is no-op because the column is being deleted > + pass > > > def recreate_comments(apps, schema_editor): Is it possible to migrate this also? Is it worth it? Stephen
On 03/23/2016 12:01 PM, Finucane, Stephen wrote: > On 17 Mar 16:04, Andy Doan wrote: >> By handling comment copying/deletion via SQL we can make migration take >> <3 minutes rather than >30minutes for big instances. The SQL is vendor >> specific, so a similar query would need to be added to support MySQL. >> >> Signed-off-by: Andy Doan <andy.doan@linaro.org> > > Some nits below, but nothing important so happy to merge. Any chance > you want to do the same for MySQL? :) I'll give it a shot. > I'd rather this check be done in each function, rather than having multiple > functions. It just seems...cleaner. will do >> def recreate_comments(apps, schema_editor): > > Is it possible to migrate this also? Is it worth it? I'm sure someone with more DB experience would have a trick, but given its only for a reverse migration I figured it wasn't worth it.
On 24 Mar 08:52, Andy Doan wrote: > On 03/23/2016 12:01 PM, Finucane, Stephen wrote: > >On 17 Mar 16:04, Andy Doan wrote: > >>By handling comment copying/deletion via SQL we can make migration take > >><3 minutes rather than >30minutes for big instances. The SQL is vendor > >>specific, so a similar query would need to be added to support MySQL. > >> > >>Signed-off-by: Andy Doan <andy.doan@linaro.org> > > > >Some nits below, but nothing important so happy to merge. Any chance > >you want to do the same for MySQL? :) > > I'll give it a shot. No panic. This can be a separate follow-up patch to get this through. > >I'd rather this check be done in each function, rather than having multiple > >functions. It just seems...cleaner. > > will do > > >> def recreate_comments(apps, schema_editor): > > > >Is it possible to migrate this also? Is it worth it? > > I'm sure someone with more DB experience would have a trick, but > given its only for a reverse migration I figured it wasn't worth it. Makes sense.
diff --git a/patchwork/migrations/0007_move_comment_content_to_patch_content.py b/patchwork/migrations/0007_move_comment_content_to_patch_content.py index 63d57ba..aea65fe 100644 --- a/patchwork/migrations/0007_move_comment_content_to_patch_content.py +++ b/patchwork/migrations/0007_move_comment_content_to_patch_content.py @@ -1,47 +1,60 @@ # -*- coding: utf-8 -*- from __future__ import unicode_literals -from django.db import migrations - - -def copy_comment_field(apps, schema_editor): - Comment = apps.get_model('patchwork', 'Comment') - Patch = apps.get_model('patchwork', 'Patch') - - for patch in Patch.objects.all(): - try: - # when available, this can only return one entry due to the - # unique_together constraint - comment = Comment.objects.get(patch=patch, msgid=patch.msgid) - except Comment.DoesNotExist: - # though there's no requirement to actually have a comment - continue - - patch.content = comment.content - patch.save() +from django.db import connection, migrations + +if connection.vendor == 'postgresql': + def copy_comment_field(apps, schema_editor): + schema_editor.execute(''' + UPDATE patchwork_patch + SET content = patchwork_comment.content + FROM patchwork_comment + WHERE patchwork_patch.id=patchwork_comment.patch_id + AND patchwork_patch.msgid=patchwork_comment.msgid + ''') + + def remove_duplicate_comments(apps, schema_editor): + schema_editor.execute(''' + DELETE FROM patchwork_comment + USING patchwork_patch + WHERE patchwork_patch.id=patchwork_comment.patch_id + AND patchwork_patch.msgid=patchwork_comment.msgid + ''') +else: + def copy_comment_field(apps, schema_editor): + Comment = apps.get_model('patchwork', 'Comment') + Patch = apps.get_model('patchwork', 'Patch') + + for patch in Patch.objects.all(): + try: + # when available, this can only return one entry due to the + # unique_together constraint + comment = Comment.objects.get(patch=patch, msgid=patch.msgid) + except Comment.DoesNotExist: + # though there's no requirement to actually have a comment + continue + + patch.content = comment.content + patch.save() + + def remove_duplicate_comments(apps, schema_editor): + Comment = apps.get_model('patchwork', 'Comment') + Patch = apps.get_model('patchwork', 'Patch') + + for patch in Patch.objects.all(): + try: + # when available, this can only return one entry due to the + # unique_together constraint + comment = Comment.objects.get(patch=patch, msgid=patch.msgid) + comment.delete() + except Comment.DoesNotExist: + # though there's no requirement to actually have a comment + continue def uncopy_comment_field(apps, schema_editor): - Patch = apps.get_model('patchwork', 'Patch') - - for patch in Patch.objects.all(): - patch.content = None - patch.save() - - -def remove_duplicate_comments(apps, schema_editor): - Comment = apps.get_model('patchwork', 'Comment') - Patch = apps.get_model('patchwork', 'Patch') - - for patch in Patch.objects.all(): - try: - # when available, this can only return one entry due to the - # unique_together constraint - comment = Comment.objects.get(patch=patch, msgid=patch.msgid) - comment.delete() - except Comment.DoesNotExist: - # though there's no requirement to actually have a comment - continue + # This is no-op because the column is being deleted + pass def recreate_comments(apps, schema_editor):
By handling comment copying/deletion via SQL we can make migration take <3 minutes rather than >30minutes for big instances. The SQL is vendor specific, so a similar query would need to be added to support MySQL. Signed-off-by: Andy Doan <andy.doan@linaro.org> --- .../0007_move_comment_content_to_patch_content.py | 89 +++++++++++++--------- 1 file changed, 51 insertions(+), 38 deletions(-)