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] Ajouter des index sur les tables pg #10441

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

Mefl
Copy link
Contributor

@Mefl Mefl commented Oct 30, 2024

🍂 Problème

Plusieurs tables postgresql sont dépourvus d'index ou ont des usages non indexés.

🎃 Remarques

Cette PR est une première partie, il reste des tables non indexés mais nous ne voulons pas ajouter tous les index d'un coup pour éviter un temps de MEP long.
Ils seront ajoutés progressivement.

🪵 Pour tester

scalingo --region osc-fr1 -a pix-api-review-pr10441 pgsql-console
\d "certification-subscriptions"
\d "complementary-certifications"
\d "features"
\d "campaign-features"
\d "complementary-certification-habilitations"
\d "complementary-certification-badges"
\d "complementary-certification-courses"
\d "organizations"
\d "organization-learners"

@Mefl Mefl requested review from Steph0 and machestla October 30, 2024 09:11
@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 :

@MathieuGilet MathieuGilet changed the title ajout d'index sur les tables pg [TECH] Ajouter des index sur les tables pg Oct 30, 2024
@xav-car xav-car added 👀 Tech Review Needed team-captains This is your captain speaking cross-team Toutes les équipes de dev labels Oct 30, 2024
@@ -0,0 +1,94 @@
const up = async function (knex) {
// table 'certification-subscriptions'
Copy link
Contributor

Choose a reason for hiding this comment

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

vu l'expressivité de l'api knex je pense qu'on peut omettre les commentaires

Copy link
Contributor

Choose a reason for hiding this comment

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

pour exprimer plus directement les tables et index ajoutés, je propose

Suggested change
// table 'certification-subscriptions'
const indexesToAdd = { 'certification-subscriptions': ['complementaryCertificationId', 'certificationCandidateId']},
{'complementary-certifications': ['key']}
;
for (const tableName of Object.keys(indexesToAdd)) {
await knex.schema.table(tableName, function (table) {
table.index(indexesToAdd[tableName]);
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai préféré une syntaxe plus classique avec un object {table:"nomTable", index: ["column de l'index"]}, mais la PR est effectivement bien plus lisible avec une vraie symétrie par construction entre le up et le down

Copy link
Contributor

@machestla machestla left a comment

Choose a reason for hiding this comment

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

Tech review ok 🎉

Copy link
Contributor

@Steph0 Steph0 left a comment

Choose a reason for hiding this comment

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

Ko au niveau des tests

J'aurais ete interesse par les EXPLAIN ANALYZE qui ont amenes choisir de positionner ces index pour mon propre apprentissage. Mais pas besoin de cela pour valider, c'est + par curiosite.

@@ -0,0 +1,28 @@
const indexesToAdd = [
{ tableName: 'certification-subscriptions', index: ['complementaryCertificationId', 'certificationCandidateId'] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Il y a deja eu un index de mis sur : api/db/migrations/20240618143134_add-index-on-certification-candidate-id-in-subscriptions-table.js

Est-ce qu'il ne faudrait pas enlever le precedent index (il n'avait pas l'air super efficace) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Il est utile mais ne couvre pas tous les usages.

{ tableName: 'certification-subscriptions', index: ['complementaryCertificationId', 'certificationCandidateId'] },
{ tableName: 'complementary-certifications', index: ['key'] },
{ tableName: 'features', index: ['key'] },
{ tableName: 'campaign-features', index: ['campaignId', 'featureId'] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Je propose d'ajouter aussi sur certification-center-features (pour grouper les tables similaires)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ca fait partie du prochain batch d'index, je ne souhaite pas ajouter plus d'index car le temps de MEP serait impacté.

@Steph0 Steph0 self-requested a review October 30, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev 👀 Tech Review Needed team-captains This is your captain speaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants