Fix checkpoint deadlock - re-use locks on the same table #13712
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ck the same table within the same transaction, and instead re-use the existing lock
github-actions bot
pushed a commit
to duckdb/duckdb-r
that referenced
this pull request
Sep 9, 2024
Merge pull request duckdb/duckdb#13712 from Mytherin/checkpointdeadlock Merge pull request duckdb/duckdb#13710 from lnkuiper/tsan_suppress_jemalloc
github-actions bot
added a commit
to duckdb/duckdb-r
that referenced
this pull request
Sep 9, 2024
Merge pull request duckdb/duckdb#13712 from Mytherin/checkpointdeadlock Merge pull request duckdb/duckdb#13710 from lnkuiper/tsan_suppress_jemalloc Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Mytherin
added a commit
that referenced
this pull request
Oct 15, 2024
…ce (#14379) This is a follow-up PR from #13712 that removes the code-paths that grabbed the table lock through a different manner, avoiding the transaction-level interface. Looking at the code this seems to only be used in the WAL - so it's unlikely this would actually cause problems, but it's better to clean up this code path entirely so it doesn't get accidentally used in the future.
Mytherin
added a commit
that referenced
this pull request
Oct 16, 2024
This is a follow-up to #13712, using a more fine-granular locking approach. #13712 has fixed an issue where checkpoints together with concurrent queries that referenced the same table multiple times could lead to deadlocks. The locking approach in that PR used a transaction-level lock to coordinate lock acquisition for the same table. In case of a query having many table scans (for different tables), this could potentially now block all executor threads if only a single table lock could not be acquired (e.g. due to a concurrent checkpoint). One thread would be blocked trying to acquire the checkpoint lock for the given table, and many others on the active_locks_lock (even though they would only be interested in acquiring the checkpoint lock for another table that wasn't being checkpointed).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where checkpoints together with concurrent queries that referenced the same table multiple times could lead to deadlocks.
The issue was as follows:
When doing a checkpoint of a table, we would grab an exclusive lock for a given table while writing the data for that table.
Grabbing an exclusive lock meant waiting for all existing readers to finish, while not allowing new readers to start. This allows existing readers to finish, while guaranteeing progress for the checkpoint thread (new readers cannot continuously block the checkpoint)
This causes problems when we have a query that refers to the same table multiple times - as we would try to grab the same lock multiple times within the same transaction. The deadlock would happen as follows:
T1grabs a read lock ontabletable, preventing new readers from startingT1tries to grab another read lock ontableThis would then result in a deadlock, as
T1would wait need to wait until the checkpoint thread finishes - but the checkpoint thread needs to wait untilT1finishes.This PR resolves the issue by keeping track of all active locks grabbed by a transaction, and ensuring we never grab the same shared lock multiple times in the same transaction.