8000 Remove duplicate index by ts-navghane · Pull Request #9510 · mautic/mautic · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove duplicate index #9510

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Mar 14, 2021
Merged

Remove duplicate index #9510

merged 21 commits into from
Mar 14, 2021

Conversation

ts-navghane
Copy link
Contributor
Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL N
Related developer documentation PR URL N
Issues addressed (#s or URLs) N
BC breaks? N
Deprecations? N

Description:

This task is to remove duplicate index (email) in email_assets_xref.

Steps to reproduce the bug:

  1. View indexes from email_assets_xref table

Steps to test this PR:

  1. Load up this PR
  2. Execute migration
  3. View indexes from email_assets_xref table

@cla-bot cla-bot bot added the cla-signed 8000 The PR contributors have signed the contributors agreement label Dec 17, 2020
@npracht npracht added bug Issues or PR's relating to bugs email Anything related to email ready-to-test PR's that are ready to test T2 Medium difficulty to fix (issue) or test (PR) labels Dec 17, 2020
@npracht npracht added this to the 3.2.2 milestone Dec 17, 2020
@codecov
Copy link
codecov bot commented Dec 17, 2020 •
edited
Loading

Codecov Report

Merging #9510 (847ac62) into 3.3 (dc009fe) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.3    #9510      +/-   ##
============================================
+ Coverage     38.73%   38.75%   +0.01%     
- Complexity    34025    34036      +11     
============================================
  Files          1988     1989       +1     
  Lines        105177   105208      +31     
============================================
+ Hits          40740    40773      +33     
+ Misses        64437    64435       -2
Impacted Files Coverage Δ Complexity Δ
...s/CoreBundle/Doctrine/Helper/IndexSchemaHelper.php 90.90% <100.00%> (+8.30%) 23.00 <7.00> (+3.00)
...e/InstallFixtures/ORM/RemoveDuplicateIndexData.php 100.00% <100.00%> (ø) 8.00 <8.00> (?)
app/bundles/LeadBundle/Form/Type/ListType.php 94.01% <0.00%> (-0.86%) 13.00% <0.00%> (ø%)

@npracht npracht modified the milestones: 3.2.2, 3.2.3 Dec 21, 2020
@npracht npracht modified the milestones: 3.2.3, 3.2.5, 3.3.1 Jan 15, 2021
@RCheesley RCheesley changed the base branch from 3.2 to features January 19, 2021 17:32
@RCheesley
Copy link
Member

@mautic/acquia-po we will need someone to rebase this one as it is on an Acquia fork rather than a personal fork. Maybe @lijupm you could take a look?

@RCheesley RCheesley added the needs-rebase PR's that need to be rebased label Jan 22, 2021
@lijupm lijupm force-pushed the remove-duplicate-index/email_assets_xref branch from d49d18e to eba584a Compare January 25, 2021 07:38
@RCheesley RCheesley added needs-automated-tests PR's that need automated tests before they can be merged and removed needs-rebase PR's that need to be rebased labels Feb 13, 2021
@RCheesley RCheesley modified the milestones: 3.3.1, 3.3.2 Feb 23, 2021
@RCheesley RCheesley changed the base branch from features to 3.3 March 3, 2021 12:21
@npracht npracht removed the needs-automated-tests PR's that need automated tests before they can be merged label Mar 4, 2021
@kuzmany kuzmany self-requested a review March 4, 2021 16:31
kuzmany
kuzmany requested changes Mar 4, 2021
View reviewed changes
Copy link
Member
@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

I don't understand this PR, what cause this issue?
I've tested migration, it remove index from email_assets_xref,

But as as I read the code, migration should cover also email_list_xref, shouldn't it?

@fedys
Copy link
Member
fedys commented Mar 5, 2021

@kuzmany you are right. The migration for email_list_xref is missing. I will cherry pick the missing migration. Thanks!

@kuzmany
Copy link
Member
kuzmany commented Mar 5, 2021

@fedys can you get me more context of issue If we have index on email column in email_assets_xref ?

@cla-bot
Copy link
cla-bot bot commented Mar 5, 2021

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @rohit3nov.

@cla-bot cla-bot bot removed the cla-signed The PR contributors have signed the contributors agreement label Mar 5, 2021
@fedys
Copy link
Member
fedys commented Mar 5, 2021

@kuzmany The index consisting of single email_id column (the last row in the screenshot) is completely redundant as DB can utilize primary key.
Snímek obrazovky 2021-03-05 v 10 23 12

@kuzmany
Copy link
Member
kuzmany commented Mar 5, 2021 •
edited
Loading

@fedys the we should remove index also for asset_id - that column is also primary

Shouldn't it? Just asking :)

@fedys
Copy link
Member
fedys commented Mar 5, 2021

@kuzmany It is not possible to remove the index for asset_id. DB is able to utilize only the first column in the compound index (primary key here) when searching/sorting by a single column.

In this scenario if you tried removing that index, DB wouldn't allow that as every foreign key needs an index.

@kuzmany
Copy link
Member
kuzmany commented Mar 5, 2021

@fedys thank you for make it clear for me. I appreciate it

kuzmany
kuzmany approved these changes Mar 5, 2021
View reviewed changes
Copy link
Member
@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Works for me 👍

@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Mar 5, 2021
@RCheesley
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Mar 5, 2021
@cla-bot
Copy link
cla-bot bot commented Mar 5, 2021

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

@npracht
Copy link
Member
npracht co 6D40 mmented Mar 5, 2021

There is a schema migration. @RCheesley can we tag it to patch release anyway or do we need to wait next minor ?

@RCheesley
Copy link
Member

As a bug fix we can go with the next patch release but let's just double check that the migration is sound before we merge.

@RCheesley RCheesley requested a review from mohit-rocks March 13, 2021 14:09
@RCheesley RCheesley merged commit 0103914 into mautic:3.3 Mar 14, 2021
@mollux mollux mentioned this pull request Jan 24, 2022
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kuzmany kuzmany kuzmany approved these changes

@mohit-rocks mohit-rocks mohit-rocks approved these changes

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement email Anything related to email pending-test-confirmation PR's that require one test before they can be merged T2 Medium difficulty to fix (issue) or test (PR)
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.
8 participants
@ts-navghane @RCheesley @fedys @kuzmany @npracht @mohit-rocks @escopecz @rohit3nov
0