Use table-level locking when acquiring shared locks #14370
Conversation
src/transaction/duck_transaction.cpp
Outdated
| return lock; | ||
| } | ||
| unique_lock<mutex> transaction_lock(active_locks_lock); | ||
| auto &active_table_lock = active_locks[info]; |
There was a problem hiding this comment.
I think this might not be safe. We're keeping a reference to an ActiveTableLock, which is an object that sits in an unordered_map. Other threads can then modify the unordered_map (and add new elements to it). My suspicion is that, if the unordered_map is resized, this can potentially blow up.
Can we turn ActiveTableLock into a unique_ptr to prevent this from happening?
There was a problem hiding this comment.
People talking about unordered_map! I think iterators can become invalid on resize, but reference to object in the std::unordered_map are always valid.
This is confirmed by:
References to elements in the unordered_map container remain valid in all cases, even after a rehash.
at https://cplusplus.com/reference/unordered_map/unordered_map/insert/
This is one of the classic complaints on std::unordered_map, that basically always require an extra level of indirection (and so it's impossible to make them faster while keeping the same interface, and changing interface to not rely on element validity might break stuff in hidden ways).
That said, making this explicit might be better.
There was a problem hiding this comment.
I've reworked this in 43237a6.
|
Thanks! |
Use table-level locking when acquiring shared locks (duckdb/duckdb#14370) Increase bounds for `test/sql/copy/file_size_bytes.test` (duckdb/duckdb#14367)
Use table-level locking when acquiring shared locks (duckdb/duckdb#14370) Increase bounds for `test/sql/copy/file_size_bytes.test` (duckdb/duckdb#14367)
Use table-level locking when acquiring shared locks (duckdb/duckdb#14370) Increase bounds for `test/sql/copy/file_size_bytes.test` (duckdb/duckdb#14367)
Use table-level locking when acquiring shared locks (duckdb/duckdb#14370) Increase bounds for `test/sql/copy/file_size_bytes.test` (duckdb/duckdb#14367)
Use table-level locking when acquiring shared locks (duckdb/duckdb#14370) Increase bounds for `test/sql/copy/file_size_bytes.test` (duckdb/duckdb#14367) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
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).