-
Notifications
You must be signed in to change notification settings - Fork 3
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
LPS-121714 Service builder should remove duplicate unique indexes #9483
Conversation
…r which ones should be kept.
To conserve resources, the PR Tester does not automatically run for every pull. If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed. If your pull was never tested, comment "ci:test" to run the PR Tester for this pull. |
ci:test:sf |
ci:test:relevant |
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-121714 1 Successful Jobs:For more details click here. |
Jenkins Build:test-portal-source-format#4144 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#3668 |
ci:forward |
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
Skipping previously passed test suites: |
All required test suite(s) passed. |
Pull request has been successfully forwarded to brianchandotcom#94826 |
Jenkins Build:test-portal-acceptance-pullrequest(master)#6991 |
@shuyangzhou, @Preston-Crary why did we implemented this? From a constraint point of view it makes sense, but from an index one they aren't redundant. For example we've removed: /cc @achaparro |
@marianoalvarosaiz if an index is needed for companyId, articlePK - add it separately. It shouldn't be a unique index if that's the reason. |
@Preston-Crary the reason is that order in index matters, and removing a simple index like the following one:
You're creating creating a performance regression in every method that uses: DLOpenerFileEntryReferencePersistenceImpl.findByR_F(String referenceType, long fileEntryId) because now we're performing a FULL SCAN with that finder. This is just an example, but with this implementation we're open to generate performance regressions when defining finders that aren't going to be backed up by database indexes. And it makes sense that some of those indexes are defined as unique for API usability reasons, to avoid having to perform afterwards a get(0) from the resulted collection. |
Here's the mysql explain for that query:
|
@Preston-Crary Sorry for the delay, I was on vacation. Unique index keys can be null. Try the following:
Now execute the explain plan in mysql:
I know that by API we don't allow nulls in Assume a custom development like the previous one with fileEntryId being a String. We could have the previous all access situation. We could have it also if the customer decides to use that filter in a custom sql. |
Hi @shuyangzhou,
Removing duplicate finders looks a bit tricky since it's not always clear to me which one should be removed and timing seems bad for major version changes. So for now just cleaning up the indexes, some unique indexes got left behind from changing constraints, those needed to be removed manually first in 28becca. I'm not sure if we need upgrades for all of these or if we can wait until the automatic index cleanup is run by future upgrades. All these changes only work for unique indexes because otherwise there are issues with ranged comparators (unique has to be
==
), see d267df9.