Développement #81814: migrate_schemas: refactoring général et optimisations - Hobo - Redmine Entr’ouvert

Actions

Copy link

Développement #81814

closed

migrate_schemas: refactoring général et optimisations

Added by Pierre Ducroquet over 2 years ago. Updated 1 day ago.

Status:

Solution déployée

Priority:

Normal

Assignee:

Benjamin Dauvergne

Category:

Target version:

Start date:

29 September 2023

Due date:

% Done:

Estimated time:

Patch proposed:

Planning:

Description

Il apparait, en lisant le code du MigrationRecorder de Django, que la fonction MigrationRecorder.has_table appelle systématiquement la méthode introspection.table_names. Cette méthode fait un appel à la base pour lister toutes les tables, ce qui devient coûteux avec notre organisation en multi-tenants (environ 20ms par appel).
https://github.com/django/django/blob/stable/4.2.x/django/db/migrations/recorder.py#L55

Étant donné que l'on vérifie au préalable l'existence de la table django_migrations, on pourrait tout simplement éliminer cette fonction pour obtenir un gain significatif à l'exécution des migrations.

Related issues 1 (0 open — 1 closed)

Actions

Copy link

Updated by Robot Gitea over 2 years ago

Tracker changed from Support to Développement
Status changed from Nouveau to En cours

Pierre Ducroquet (pducroquet) a ouvert une pull request sur Gitea concernant cette demande :

URL : https://git.entrouvert.org/entrouvert/hobo/pulls/78
Titre : WIP: multitenant: hook MigrationRecorder::has_table for a speedup (#81814)
Modifications : https://git.entrouvert.org/entrouvert/hobo/pulls/78/files

Actions

Copy link

Updated by Robot Gitea over 2 years ago

Status changed from En cours to Solution proposée

Actions

Copy link

Updated by Thomas Noël over 2 years ago

J'ai testé, avant et après le patch, une migration où je n'ai changé que la longueur d'un champ inutilisé :

--- a/passerelle/apps/soap/models.py
+++ b/passerelle/apps/soap/models.py
@@ -35,7 +35,7 @@ from passerelle.utils.jsonresponse import APIError

 class SOAPConnector(BaseResource, HTTPResource):
     wsdl_url = models.URLField(
-        max_length=400, verbose_name=_('WSDL URL'), help_text=_('URL of the WSDL file')
+        max_length=401, verbose_name=_('WSDL URL'), help_text=_('URL of the WSDL file')
     )
     zeep_strict = models.BooleanField(default=False, verbose_name=_('Be strict with returned XML'))
     zeep_xsd_ignore_sequence_order = models.BooleanField(

Lancement de migrate_schemas sur 50 tenants :

sans le patch : 1m32,351s
avec le patch : 1m30,653s

Je ne sais pas trop calculer la part de risque du patch par rapport à ce gain de 2% qui ne va pas changer grand chose en pratique (attendre 19 minutes contre 20 actuellement).

J'ai envie d'être conservateur...

Actions

Copy link

Updated by Benjamin Dauvergne about 2 years ago

Ça fera gagner du temps sur les restart (qui prendront 8 secondes de moins pour 200 tenants donc) en fait pas tellement sur les migrations du jeudi soir. Par contre ce has_table n'est pas juste inutile, migrate_schemas est aussi utilisé pour initialiser un schéma (dans Tenant.create_schema upstream dans django-tenant-schemas) et dans ce cas on veut créer la table des migrations.

Actions

Copy link

Updated by Robot Gitea about 2 years ago

Status changed from Solution proposée to En cours

Benjamin Dauvergne (bdauvergne) a relu et demandé des modifications sur une pull request sur Gitea concernant cette demande :

URL : https://git.entrouvert.org/entrouvert/hobo/pulls/78

Actions

Copy link

Updated by Benjamin Dauvergne about 1 month ago

Assignee changed from Pierre Ducroquet to Benjamin Dauvergne

Actions

Copy link

Updated by Benjamin Dauvergne about 1 month ago

🤖 Une pull request concernant ce ticket a été ouverte :

URL : https://git.entrouvert.org/entrouvert/hobo/pulls/311
Titre : WIP: Éviter de lire la liste des tables dans MigrationRecord quand on sait déjà que la table django_migrations existe (#81814)
Modifications : https://git.entrouvert.org/entrouvert/hobo/pulls/311/files

Actions

Copy link

Updated by Benjamin Dauvergne about 1 month ago

🤖 Pull request fermée.

Actions

Copy link

Updated by Benjamin Dauvergne about 1 month ago

Status changed from En cours to Solution proposée

Actions

Copy link

#10

Updated by Benjamin Dauvergne about 1 month ago

Subject changed from migrate_schemas: surcharger MigrationRecorder.has_table pour accélérer les migrations to migrate_schemas: refactoring général et optimisations

Je suis allé finalement plus loin que le patch de base, il y a toujours l'optimisation sur MigrationRecorded.has_table, mais en plus je corrige un dernier point du calcul de l'ETA: on utilisait toujours le nombre de tenants totaux pour calculer le temps total, désormais on découpe l'application des migrations en deux :

d'abord lister les tenants ciblés,
ensuite appliquer les migrations.

Au passage je corrige des défauts dans la récupération des exceptions dans get_applied_migrations(), et aussi le retour sur le schéma public qui n'était pas toujours fait correctement et/ou au bon endroit et je mutualise run_migrations() entre le 3 cas (un tenant, un schéma, tous les tenants). Au final je trouve ça plus lisible.

Actions

Copy link

#11