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 lock order when dropping index #7600

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Jan 17, 2025

If an index is dropped, it is necessary to lock the heap table (of
the index) before the index since all normal operations do it in this
order. When dropping an index, we did not take all the necessary locks
in the right order before calling performMultipleDeletions, which can
cause deadlocks when dropping an index on a hypertable at the same time
as running a utility statement that takes heavy locks, e.g., VACUUM or
ANALYZE.

Adding a isolation test as well that will generate a deadlock if the
index and table locks are not taken in the correct order.

@mkindahl mkindahl force-pushed the fix-index-deadlock branch 2 times, most recently from 1f7a183 to 951b92a Compare January 17, 2025 10:04
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.32%. Comparing base (59f50f2) to head (e584632).
Report is 720 commits behind head on main.

Files with missing lines Patch % Lines
src/chunk_index.c 50.00% 3 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7600      +/-   ##
==========================================
+ Coverage   80.06%   81.32%   +1.25%     
==========================================
  Files         190      240      +50     
  Lines       37181    44708    +7527     
  Branches     9450    11164    +1714     
==========================================
+ Hits        29770    36359    +6589     
- Misses       2997     3960     +963     
+ Partials     4414     4389      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkindahl mkindahl marked this pull request as ready for review January 17, 2025 10:58
src/chunk_index.c Outdated Show resolved Hide resolved
@mkindahl mkindahl requested a review from erimatnor January 29, 2025 09:39
@mkindahl mkindahl force-pushed the fix-index-deadlock branch 2 times, most recently from a7bd859 to 8f0c997 Compare January 30, 2025 07:39
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

Just nits

tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
tsl/test/isolation/specs/deadlock_drop_index_vacuum.spec Outdated Show resolved Hide resolved
If an index is dropped, it is necessary to lock the heap table (of
the index) before the index since all normal operations do it in this
order. When dropping an index, we did not take all the necessary locks
in the right order before calling `performMultipleDeletions`, which can
cause deadlocks when dropping an index on a hypertable at the same time
as running a utility statement that takes heavy locks, e.g., VACUUM or
ANALYZE.

Adding a isolation test as well that will generate a deadlock if the
index and table locks are not taken in the correct order.
Copy link
Contributor

@antekresic antekresic left a comment

Choose a reason for hiding this comment

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

lgtm

@mkindahl mkindahl merged commit 538c443 into timescale:main Jan 30, 2025
51 of 53 checks passed
@mkindahl mkindahl deleted the fix-index-deadlock branch January 30, 2025 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants