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

Fix #238: migration to backport prod index #979

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

leplatrem
Copy link
Contributor

Fix #238

@leplatrem leplatrem requested a review from a team as a code owner October 1, 2024 17:16
@leplatrem leplatrem marked this pull request as draft October 1, 2024 17:16
@leplatrem leplatrem added the enhancement New feature or request label Oct 1, 2024
@leplatrem leplatrem force-pushed the fix-migration-backport branch from 8e8d4d4 to 15d461c Compare October 2, 2024 08:16
@leplatrem leplatrem marked this pull request as ready for review October 2, 2024 08:16
@grahamalama
Copy link
Contributor

grahamalama commented Oct 7, 2024

Trying to get the timeline correct here:

    op.create_index(
        "idx_email_primary_email_lower",
        "emails",
        [sa.text("lower(primary_email)")],
        unique=True,
    )

so functionally the same as the one we have in this PR, just a different name

I don't know how / when bsieber-mozilla's migrations were applied in prod, but maybe the first version of the migration was applied and ran, but the edit wasn't picked up 🤔. Pure speculation.

Copy link
Contributor

@grahamalama grahamalama left a comment

Choose a reason for hiding this comment

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

But in any case, some thoughts / questions:

  • is it true that we have an index idx_email_primary_email_lower, and now we're adding this idx_email_primary_unique_email_lower? Maybe we can drop idx_email_primary_email_lower since idx_email_primary_unique_email_lower is that plus uniqueness

  • if we do need this index, can we include a comment here explaining why, given the confusing history I pointed out in my previous comment?

  • Can we reflect this index back onto the model?

@leplatrem
Copy link
Contributor Author

leplatrem commented Oct 8, 2024

You are perfectly right! I was only looking at diffs between schemas, and didn't spot that we also have idx_email_primary_email_lower.

Stage/Prod have this additional one idx_email_primary_unique_email_lower.

I propose that we keep the one with uniqueness, and drop the other one. Let's see how I can rework the migration for that.

Thank you for being so thorough, as usual 🙏

@leplatrem leplatrem requested a review from grahamalama October 8, 2024 13:51
Copy link
Contributor

@grahamalama grahamalama left a comment

Choose a reason for hiding this comment

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

Nice! One topic of investigation for indexes that I though of, but otherwise looks g2g and I'll throw the approval on now.

ctms/models.py Show resolved Hide resolved
@leplatrem leplatrem merged commit b03cd6d into main Oct 10, 2024
5 checks passed
@leplatrem leplatrem deleted the fix-migration-backport branch October 10, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an alembic migration to sync with production indexes
2 participants