Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TECH] Ajout d'un index sur la table certification-subscriptions (PIX-13028). #9312

Conversation

alexandrecoin
Copy link
Contributor

@alexandrecoin alexandrecoin commented Jun 18, 2024

🦄 Problème

Suite à une précèdente migration dans une PR précèdente #9228
Les appels de l'espace surveillant (toutes les 5 secondes) ont déclenchés une sur-sollicitation de la base de données en production, crééant un ralentissement de la plateforme.

Un rollback de la migration a été déclenché pour correction.

🤖 Proposition

  • EXPLAIN ANALYZE de la requête problématique
    • ⚠️ sur une base de données locale chargée avec un gros volume de candidats (9 Million), sans quoi le QUERY PLAN serait faussé
  • Mise en place d'un index
  • Nouvel EXPLAIN ANALYZE pour vérifier l'impact

🌈 Remarques

Le EXPLAIN ANALYZE a révélé la chose suivante sur la requête de supervision

->  Parallel Seq Scan on "complementary-certification-subscriptions"  (cost=0.00..101779.62 rows=1 width=8) (actual time=189.316..298.650 rows=4 loops=3)                  |
                                                  Filter: ("complementaryCertificationId" IS NOT NULL)                                                                                                                 |
                                                  Rows Removed by Filter: 3220017

Basé sur les critères résumé ci-dessous

Find the nodes where most of the execution time was spent.

Find the lowest node where the estimated row count is significantly different from the actual row count. Very often, this is the cause of bad performance, and the long execution time somewhere else is only a consequence of a bad plan choice based on a bad estimate. “Significantly different” typically means a factor of 10 or so.

Find long running sequential scans with a filter condition that removes many rows. These are good candidates for an index.

source : How to interpret PostgreSQL EXPLAIN ANALYZE output

Si on confronte au EXPLAIN ANALYZE et particulièrement au snippet précèdent

  • C’est un seq scan
  • Pire : un parallel seq scan : il décide tente de séparer le volume de données en deux pour trouver + vite
  • C’est le second “plus long” en execution: 298.650
  • Le filter enlève un nombre très élevé de lignes : Rows Removed by Filter: 3220017

💯 Pour tester

A voir si + de tests sur un environnemen t dédié, en tout cas en local

Avant / Après index

⚠️ Test non réalisé sur un environnement de benchmark dédié

perf_test

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@Steph0 Steph0 force-pushed the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch from 01f83dc to 22d0dad Compare June 19, 2024 07:49
@Steph0 Steph0 marked this pull request as ready for review June 19, 2024 07:50
Copy link
Contributor

@aceol aceol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'aurais mis ca sur 2 migrations, mais OK 👌

@aceol aceol force-pushed the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch from 22d0dad to 141093c Compare June 19, 2024 08:58
@Steph0 Steph0 force-pushed the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch from 141093c to 20101eb Compare June 19, 2024 09:41
@Steph0
Copy link
Contributor

Steph0 commented Jun 19, 2024

Test ✔️

Sur une base de 9 millions de certification-candidates associés à 9 millions de CORE, avec des sessionId random, répartis une peu partout dans certification-candidates ; j'ai joué en faisant des npm db:migrate et npm db:rollback:latest, et on ça valide : l'impact de l'index et que le rollback est OK.

image

@Steph0 Steph0 added Func Review OK PO validated functionally the PR and removed 👀 Func Review Needed labels Jun 19, 2024
@Steph0 Steph0 requested a review from MathieuGilet June 19, 2024 10:16
@Steph0 Steph0 force-pushed the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch 3 times, most recently from 3156213 to 7e6cbf4 Compare June 19, 2024 11:44
@alexandrecoin
Copy link
Contributor Author

Est-ce qu'on ne veut pas inverser les commits afin d'ajouter l'index une fois les lignes créées ?

@MathieuGilet
Copy link
Contributor

MathieuGilet commented Jun 19, 2024

C'est une excellente remarque. Je me demande si la date de création du fichier (la première partie du nom) ne suffit pas à faire en sorte que l'index sera créé après les insertions.

Je viens de tester en local, ce sera le bon ordre, insert puis création de l'index.

@alexandrecoin
Copy link
Contributor Author

C'est une excellente remarque.

C'était la tienne de base, je n'ai fait que la reprendre.
Rendons à César ce qui appartient à César.

@pix-service-auto-merge pix-service-auto-merge force-pushed the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch from 7e6cbf4 to f5abc22 Compare June 20, 2024 07:58
@pix-service-auto-merge pix-service-auto-merge merged commit 5eba397 into dev Jun 20, 2024
6 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-13028-add-index-on-certification-candidate-id-in-subscriptions-table branch June 20, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants