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

vinyl: fix index name in duplicate key error message #10023

Merged
merged 1 commit into from
May 20, 2024

Conversation

locker
Copy link
Member

@locker locker commented May 17, 2024

The code setting ER_TUPLE_FOUND uses index_name_by_id() to find the index name, but it passes an index in the dense index map to it while the function expects an index in the sparse index map. Apparently, this doesn't work as expected after an index is removed from the middle of the index map. This bug was introduced by commit fc3834c.

Instead of just fixing the index passed to index_name_by_id(), we do a bit of refactoring. We stop passing index_name and space_name to vy_check_is_unique_*() functions and instead get them right before raising ER_TUPLE_FOUND. Note, to get the space name, we need to call space_by_id() but it should be fine because (a) the space is very likely to be cached as the last accessed one and (b) this is an error path so it isn't performance critical. We also drop index_name_by_id() and extract the index name from the LSM tree object.

Closes #5975

The code setting ER_TUPLE_FOUND uses index_name_by_id() to find
the index name, but it passes an index in the dense index map to
it while the function expects an index in the sparse index map.
Apparently, this doesn't work as expected after an index is removed
from the middle of the index map. This bug was introduced by
commit fc3834c ("vinyl: check key uniqueness before modifying
tx write set").

Instead of just fixing the index passed to index_name_by_id(), we do
a bit of refactoring. We stop passing index_name and space_name to
vy_check_is_unique_*() functions and instead get them right before
raising ER_TUPLE_FOUND. Note, to get the space name, we need to call
space_by_id() but it should be fine because (a) the space is very likely
to be cached as the last accessed one and (b) this is an error path so
it isn't performance critical. We also drop index_name_by_id() and
extract the index name from the LSM tree object.

Closes #5975

NO_DOC=bug fix
@locker locker requested a review from a team as a code owner May 17, 2024 09:48
@coveralls
Copy link

Coverage Status

coverage: 87.084% (-0.01%) from 87.097%
when pulling eb28d48 on vy-duplicate-key-error-fix
into ef30ca9
on master
.

@locker locker requested a review from nshy May 17, 2024 10:09
@locker locker assigned locker and unassigned nshy May 17, 2024
@locker locker added the full-ci Enables all tests for a pull request label May 20, 2024
@locker
Copy link
Member Author

locker commented May 20, 2024

The integration/php-queue test failure isn't caused by this fix.

@locker locker merged commit 2cfba5e into master May 20, 2024
93 of 94 checks passed
@locker locker deleted the vy-duplicate-key-error-fix branch May 20, 2024 11:42
@locker
Copy link
Member Author

locker commented May 20, 2024

Cherry-picked to 2.11 and 3.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate-key error message for vinyl says index name is null
4 participants