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

Add per-table locking for AutoIncrementTracker #7699

Merged
merged 2 commits into from Apr 9, 2024

Conversation

nicktobey
Copy link
Contributor

@nicktobey nicktobey commented Apr 5, 2024

This PR refactors the AutoIncrementTracker to hold a separate mutex for each table instead of a single mutex for the entire database.

Why are we doing this?

This relates to supporting the innodb_autoinc_lock_mode variable, described here: #7634

Currently, the engine acquires the AutoIncrementTracker lock once per inserted row and holds it only while that row is being inserted. However, when innodb_autoinc_lock_mode is set to any value other than its default, the engine must instead hold the lock for the duration of the insert statement.

Unfortunately, there is a specific corner case that causes innodb_autoinc_lock_mode to interact poorly with a single global lock: if a table has a trigger, and that trigger attempts to insert into a different table, then a second insert statement begins executing before the first insert statement completes. In this case, the engine may attempt to acquire the global lock while it's already being held.

There are four possible ways to fix this:
1 - Use reentrant/recursive locks
2 - Analyze the entire execution plan to determine whether it will require the lock, and then acquire the lock at most once.
3 - Add additional logic to determine whether a session already holds the lock and avoid reacquiring it
4 - Use per-table locks

Option 1 is controversial: golang does not support recursive mutexes by design, and their use is discouraged by many. Option 2 will hold the lock for longer than is required, which would negatively impact performance. Option 3 is error prone and may result in deadlocks if implemented incorrectly.

Thus, option 4 is the best option.

What is the impact of this PR?

Concurrent inserts into two different tables with auto-increment columns should be faster because of reduced lock contention, at the cost of the additional overhead of managing multiple mutexes when the inserts are not concurrent. The magnitude of this impact is difficult to predict or measure, but is likely overall positive.

Why are there no tests?

This change should have no observable effect on query results. Our existing integration test suite is comprehensive enough that if no existing integration tests break, this should be safe.

@nicktobey nicktobey requested a review from zachmu April 5, 2024 16:59
@nicktobey
Copy link
Contributor Author

We achieve per-table locking by managing a map from table names to mutexes, as well as using a synchronized map for storing the auto-increment counters. Both of these are necessary:

  • We require a synchronized map because concurrent accesses to normal golang maps are not thread-safe, since they may trigger a re-allocation.
  • We require the mutex map because some of the operations we perform on the counter map are not atomic. Thus, before any operation that reads or writes to the auto-increment value of a table, we acquire the lock corresponding to that table.

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
fc32126 ok 5937457
version total_tests
fc32126 5937457
correctness_percentage
100.0

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM, elegant solution

go/libraries/doltcore/sqle/dsess/mutexmap/mutexmap.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/dsess/autoincrement_tracker.go Outdated Show resolved Hide resolved
nicktobey added a commit that referenced this pull request Apr 5, 2024
@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
262a397 ok 5937457
version total_tests
262a397 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
4102696 ok 5937457
version total_tests
4102696 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
45493be ok 5937457
version total_tests
45493be 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
fef14f3 ok 5937457
version total_tests
fef14f3 5937457
correctness_percentage
100.0

@nicktobey nicktobey merged commit 02b3213 into main Apr 9, 2024
20 checks passed
@nicktobey nicktobey deleted the nicktobey/pertablelocking branch April 9, 2024 21:18
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.

None yet

3 participants