Produits Entr'ouvert » w.c.s.

Ce patch change l'ensemble des requêtes SQL de wcs sur les formdata. J'aurais aimé être averti pour évaluer l'impact de ce changement, plutôt que découvrir après la mise en prod l'ensemble des requêtes impactées, y compris certaines étant utilisées par l'interface web et donc dont les performances doivent être maintenues.
Qu'est-ce-qui justifie d'altérer ainsi l'ensemble des requêtes ?

Pierre Ducroquet a écrit :

Ce patch change l'ensemble des requêtes SQL de wcs sur les formdata. J'aurais aimé être averti pour évaluer l'impact de ce changement, plutôt que découvrir après la mise en prod l'ensemble des requêtes impactées, y compris certaines étant utilisées par l'interface web et donc dont les performances doivent être maintenues.

Noté, j'ai pas imaginé qu'ajouter un critère IS NULL aux requêtes pouvait avoir un impact notable sur les perfs.

Qu'est-ce-qui justifie d'altérer ainsi l'ensemble des requêtes ?

Étendre les possibilité de l'infra de test de formulaire/workflow.

Valentin Deniaud a écrit :

Pierre Ducroquet a écrit :

Ce patch change l'ensemble des requêtes SQL de wcs sur les formdata. J'aurais aimé être averti pour évaluer l'impact de ce changement, plutôt que découvrir après la mise en prod l'ensemble des requêtes impactées, y compris certaines étant utilisées par l'interface web et donc dont les performances doivent être maintenues.

Noté, j'ai pas imaginé qu'ajouter un critère IS NULL aux requêtes pouvait avoir un impact notable sur les perfs.

En piochant juste dans les logs, j'ai déjà un cas qui double son temps d'exécution.
Modifier l'intégralité des requêtes d'une application est un changement majeur, ce n'est jamais à faire à la légère.

Qu'est-ce-qui justifie d'altérer ainsi l'ensemble des requêtes ?

Étendre les possibilité de l'infra de test de formulaire/workflow.

Je ne comprends pas cette réponse, en quoi cela nécessite de changer toutes les requêtes ?

Pierre Ducroquet a écrit :

Étendre les possibilité de l'infra de test de formulaire/workflow.

Je ne comprends pas cette réponse, en quoi cela nécessite de changer toutes les requêtes ?

Ben il faut lire la description du ticket/les tickets liés : ça a amené à la conclusion que pour tester des cas complexes il fallait stocker les demandes issues de l'exécution des tests, pour faire ça le plus simple est de les avoir dans la même table, avec un flag qui les différencie, d'ailleurs c'est un principe qui fonctionne déjà pour les utilisateurs. Ces demandes sont ensuite nettoyées régulièrement via cron.

Si jamais il y a une issue facile via un index (pour info, si ça peut aider, et a priori, la grosse majorité des lignes devraient être à NULL pour cette colonne) ça peut bien sûr être géré et rapidement déployé en hotfix.

Frédéric Péters a écrit :

Si jamais il y a une issue facile via un index (pour info, si ça peut aider, et a priori, la grosse majorité des lignes devraient être à NULL pour cette colonne) ça peut bien sûr être géré et rapidement déployé en hotfix.

Le problème c'est que cela impacte tous les index et toutes les opérations.

Prenons un cas basique : SELECT id FROM formdata_42 WHERE status <> 'draft' ORDER BY id DESC;, qui prend 2,5s pour allotoulouse, et qui passe à 3,5s quand on y ajoute cette nouvelle clause, alors que dans les deux cas on ne fait que scanner la clé primaire.

Et ce n'est pas le pire cas, on perd aussi pas mal d'index only scan qui deviennent des index scan, et donc beaucoup plus lents pour des gros volumes.

Je ne comprends toujours pas pourquoi cela doit être fait en altérant ainsi l'ensemble de l'application. Je vois des cas où cela n'a aucun intérêt d'avoir ce filtrage (par exemple pour déterminer les valeurs distinctes d'un champ pour lister les valeurs des filtres dans l'interface graphique), donc je ne comprends pas pourquoi avoir fait ce choix.
Suggestion sans avoir l'ensemble du code en tête : si ces données n'ont vocation à n'être utilisées que dans des cas très limités, alors il ne faut pas les mettre dans les tables elle-même et plutôt altérer SqlDataMixin pour utiliser une autre table quand on en a le besoin. Il n'y aura pas de solution magique dans PostgreSQL pour le comportement introduit ici, puisque même les index des clés primaires souffrent de ce choix.

Pierre Ducroquet a écrit :

Je ne comprends toujours pas pourquoi cela doit être fait en altérant ainsi l'ensemble de l'application.

C'est très important de ne pas mélanger les vraies données et les données de test.

Je vois des cas où cela n'a aucun intérêt d'avoir ce filtrage (par exemple pour déterminer les valeurs distinctes d'un champ pour lister les valeurs des filtres dans l'interface graphique)

J'y vois totalement un intérêt, des valeurs qui existeraient uniquement côté demandes issues des tests n'auront pas leur place ici.

Suggestion sans avoir l'ensemble du code en tête : si ces données n'ont vocation à n'être utilisées que dans des cas très limités, alors il ne faut pas les mettre dans les tables elle-même et plutôt altérer SqlDataMixin pour utiliser une autre table quand on en a le besoin.

C'est peut-être jouable mais ça signifie multiplier par deux le nombre de tables formdata/carddata, pourtant de ce que j'en sais il est déjà conséquent ?

Valentin Deniaud a écrit :

Pierre Ducroquet a écrit :

Je ne comprends toujours pas pourquoi cela doit être fait en altérant ainsi l'ensemble de l'application.

C'est très important de ne pas mélanger les vraies données et les données de test.

C'est pourtant exactement ce que tu fais quand tu choisis de stocker dans la même table les données de test et les vraies données, en espérant filtrer partout dans le code ensuite.

Suggestion sans avoir l'ensemble du code en tête : si ces données n'ont vocation à n'être utilisées que dans des cas très limités, alors il ne faut pas les mettre dans les tables elle-même et plutôt altérer SqlDataMixin pour utiliser une autre table quand on en a le besoin.

C'est peut-être jouable mais ça signifie multiplier par deux le nombre de tables formdata/carddata, pourtant de ce que j'en sais il est déjà conséquent ?

Mieux vaut multiplier par deux les tables (et encore, je pense que les tables devraient n'exister que quand il y a des données de test, de ce que je vois c'est rarissime aujourd'hui) plutôt que d'avoir à refaire toute l'indexation et espérer ne pas avoir de cas où le filtrage n'est pas appliqué.

Pierre Ducroquet a écrit :

refaire toute l'indexation

Tu pourrais détailler ce qu'implique cette partie ?

On a maintenant assez de recul pour avoir dans les statistiques de pgbouncer l'impact de ce changement sur le temps passé en requêtes SQL.

Date    Front1    Front2
2024-09-15    709    1296
2024-09-16    752    1044
2024-09-17    763    1023
2024-09-18    751    1024
2024-09-19    751    1063
2024-09-20    735    966
2024-09-21    722    1133
2024-09-22    726    1302
2024-09-23    751    1368
2024-09-24    770    982
2024-09-25    795    895
2024-09-26    896    1101
2024-09-27    940    1553
2024-09-28    1004    1327
2024-09-29    979    1286
2024-09-30    1040    1263
2024-10-01    1052    1309
2024-10-02    1117    1265
2024-10-03    1036    1223
2024-10-04    1011    1263
2024-10-05    1029    1528
2024-10-06    1160    1964
2024-10-07    1589    1351

On voit nettement à la mise en prod du 26 le changement sur le temps passé, en particulier sur front1 à cause des crons qui font énormément de requêtes basiques qui subissent plus les conséquences du changement des requêtes.

Valentin Deniaud a écrit :

Pierre Ducroquet a écrit :

refaire toute l'indexation

Tu pourrais détailler ce qu'implique cette partie ?

Recréer tous les index de la base avec un filtre reprenant la clause, quand c'est possible.

Automatic expiration