Skip to content

vinyl: fix crash on extending secondary key parts with primary#10101

Merged
locker merged 1 commit intotarantool:masterfrom
locker:vy-index-update-def-fix
Jun 7, 2024
Merged

vinyl: fix crash on extending secondary key parts with primary#10101
locker merged 1 commit intotarantool:masterfrom
locker:vy-index-update-def-fix

Conversation

@locker
Copy link
Member

@locker locker commented Jun 6, 2024

If a secondary index is altered in such a way that its key parts are extended with the primary key parts, rebuild isn't required because cmp_def doesn't change, see vinyl_index_def_change_requires_rebuild. In this case vinyl_index_update_def will try to update key_def and cmp_def in-place with key_def_copy. This will lead to a crash because the number of parts in the new key_def is greater.

We can't use key_def_dup instead of key_def_copy there because there may be read iterators using the old key_def by pointer so there's no other option but to force rebuild in this case.

The bug was introduced in commit 6481706.

Closes #10095

@locker locker requested a review from a team as a code owner June 6, 2024 08:37
@coveralls
Copy link

Coverage Status

coverage: 87.063% (-0.04%) from 87.107%
when pulling 09e18ba on locker:vy-index-update-def-fix
into 97a801e
on tarantool:master
.

@locker locker requested a review from nshy June 6, 2024 09:38
@nshy nshy assigned locker and unassigned nshy Jun 7, 2024
@locker locker force-pushed the vy-index-update-def-fix branch from 09e18ba to 346661a Compare June 7, 2024 08:33
@locker locker added the full-ci Enables all tests for a pull request label Jun 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 87.086% (-0.02%) from 87.101%
when pulling 346661a on locker:vy-index-update-def-fix
into 822aedf
on tarantool:master
.

@locker locker removed the full-ci Enables all tests for a pull request label Jun 7, 2024
@locker
Copy link
Member Author

locker commented Jun 7, 2024

Using key_def_dup instead of key_def_copy in vinyl_index_update_def is wrong because there may be read iterators that use the old key definition object by pointer. Looks like the only option is to force rebuild if secondary index key parts are updated with primary index key parts, though it may seem pointless as the sorting order doesn't change in this case. I updated the patch and the commit description appropriately (the test and release notes didn't change).

@locker locker force-pushed the vy-index-update-def-fix branch from 346661a to 6d4fb9f Compare June 7, 2024 10:03
@locker locker requested a review from nshy June 7, 2024 10:03
@locker locker assigned nshy and unassigned locker Jun 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 87.108% (-0.01%) from 87.119%
when pulling 6d4fb9f on locker:vy-index-update-def-fix
into bc0daf9
on tarantool:master
.

@nshy nshy assigned locker and unassigned nshy Jun 7, 2024
If a secondary index is altered in such a way that its key parts are
extended with the primary key parts, rebuild isn't required because
`cmp_def` doesn't change, see `vinyl_index_def_change_requires_rebuild`.
In this case `vinyl_index_update_def` will try to update `key_def` and
`cmp_def` in-place with `key_def_copy`. This will lead to a crash
because the number of parts in the new `key_def` is greater.

We can't use `key_def_dup` instead of `key_def_copy` there because
there may be read iterators using the old `key_def` by pointer so
there's no other option but to force rebuild in this case.

The bug was introduced in commit 6481706 ("vinyl: use update_def
index method to update vy_lsm on ddl").

Closes tarantool#10095

NO_DOC=bug fix
@locker locker force-pushed the vy-index-update-def-fix branch from 6d4fb9f to e3c433f Compare June 7, 2024 11:45
@locker locker added the full-ci Enables all tests for a pull request label Jun 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 87.095% (+0.02%) from 87.077%
when pulling e3c433f on locker:vy-index-update-def-fix
into bde28f0
on tarantool:master
.

@locker locker merged commit 9b81784 into tarantool:master Jun 7, 2024
@locker locker deleted the vy-index-update-def-fix branch June 7, 2024 12:35
@locker
Copy link
Member Author

locker commented Jun 7, 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.

tarantool: ./src/box/key_def.c:136: key_def_copy: Assertion `sz == key_def_copy_size(dest)' failed.

4 participants