Development #66315
Fonction sql::SqlDataMixin.rebuild_security trop aggressive
0%
Description
Dans une transaction, elle va réaliser un UPDATE inconditionnel sur chaque formdata d'un formdef.
Avec l'introduction de wcs_all_forms sous forme de table maintenue par triggers, ce comportement est exacerbé et peut aboutir à des deadlock, en particulier sur les instances où le serveur PostgreSQL n'a pas des disques aussi bons que les notres.
Je vois donc deux améliorations à faire sur cette fonction :
- Ne pas mettre à jour inutilement, en ajoutant des critères sur l'UPDATE.
- Étant donné le code, je pense que l'on peut faire des COMMIT intermédiaires sans craintes, par exemple un commit tous les 100 objets modifiés.
Le second correctif a l'avantage de réduire considérablement la durée des locks qui seront pris par la fonction. Actuellement, toutes les lignes se retrouvent verrouillées pour toute la durée de l'opération.
Fichiers
Demandes liées
Révisions associées
Historique
Mis à jour par Pierre Ducroquet il y a presque 2 ans
- Lié à Bug #65416: TransactionRollbackError: ERREUR: Bloquage mortel détecté ajouté
Mis à jour par Pierre Ducroquet il y a presque 2 ans
- Fichier 0001-sql-rework-the-rebuild_security-function-66315.patch 0001-sql-rework-the-rebuild_security-function-66315.patch ajouté
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
Ci-joint, un patch qui fait le taf a priori:
1) les update ne sont plus systématiques
2) en cours d'update, des commits sont faits périodiquement
Mis à jour par Frédéric Péters il y a presque 2 ans
L'approche minimaliste ici me convient tout à fait.
Sur la forme on fera plutôt
for i, formdata in enumrate(formdatas):
Aussi must_update est bizarrement nommée dans la mesure où True ou False il y a un UPDATE dans les deux cas; ça serait il me semble mieux en étant renommé en quelque chose comme update_all.
Mis à jour par Benjamin Dauvergne il y a presque 2 ans
Dans le cas must_update=False, vu qu'on a chargé le formdata est-ce qu'on ne pourrait pas complètement éviter la requête UPDATE en comparant directement la cible avec ce qui a été chargé ?
Mis à jour par Pierre Ducroquet il y a presque 2 ans
Ci-joint un patch mis à jour.
Par contre lors du rebase sur origin/main j'ai une erreur qui est apparue qui ne me semble pas liée à ce patch.
J'ai renommé le must_update en update_all, comme suggéré. Pour la suggestion de faire la vérification côté Python, je n'ai pas réussi à obtenir un résultat satisfaisant, et ça complique le code. Donc je pense qu'on peut se contenter du patch actuel.
Mis à jour par Pierre Ducroquet il y a presque 2 ans
Mis à jour par Frédéric Péters il y a presque 2 ans
Par contre lors du rebase sur origin/main j'ai une erreur qui est apparue qui ne me semble pas liée à ce patch.
N'hésite pas dans ces situations à lancer un nouveau build (ça peut juste être jenkins un peu trop chargé et des tests qui dépendent du timing).
Mis à jour par Pierre Ducroquet il y a presque 2 ans
Frédéric Péters a écrit :
Par contre lors du rebase sur origin/main j'ai une erreur qui est apparue qui ne me semble pas liée à ce patch.
N'hésite pas dans ces situations à lancer un nouveau build (ça peut juste être jenkins un peu trop chargé et des tests qui dépendent du timing).
En effet, c'était juste ça, j'ignorais qu'on avait des tests dépendant du timing
Mis à jour par Benjamin Dauvergne il y a presque 2 ans
- Statut changé de Solution proposée à Solution validée
Dans wcs/formdata.py le paramètre force_update s'appelle toujours comme ça. Sinon c'est bon.
Mis à jour par Pierre Ducroquet il y a presque 2 ans
- Statut changé de Solution validée à Résolu (à déployer)
Merci, mergé (avec la correction du nom de paramètre dans formdata.py)
commit 0c08fc01ecf8413946ddb2e899dcb16f970132bd (HEAD -> main, origin/main, origin/HEAD, wip/66315-lighten-rebuild_security)
Author: Pierre Ducroquet <pducroquet@entrouvert.com>
Date: Thu Jun 16 12:32:36 2022 +0200
sql: rework the rebuild_security function (#66315)
The function rewrote the whole table without checking if updates were really required.
Instead, we now check before updating, and we commit every 100 rows to prevent any
long lock
Mis à jour par Transition automatique il y a presque 2 ans
- Statut changé de Résolu (à déployer) à Solution déployée
sql: rework the rebuild_security function (#66315)
The function rewrote the whole table without checking if updates were really required.
Instead, we now check before updating, and we commit every 100 rows to prevent any
long lock