-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add blocking advisory locks with deadlock detection for PostgreSQL #140
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,11 +10,23 @@ module PostgreSQLAdvisory | |||||||||
| LOCK_RESULT_VALUES = ['t', true].freeze | ||||||||||
| ERROR_MESSAGE_REGEX = / ERROR: +current transaction is aborted,/ | ||||||||||
|
|
||||||||||
| def try_advisory_lock(lock_keys, lock_name:, shared:, transaction:, timeout_seconds: nil) | ||||||||||
| def try_advisory_lock(lock_keys, lock_name:, shared:, transaction:, timeout_seconds: nil, blocking: false) | ||||||||||
| # timeout_seconds is accepted for compatibility but ignored - PostgreSQL doesn't support | ||||||||||
| # native timeouts with pg_try_advisory_lock, requiring Ruby-level polling instead | ||||||||||
|
Comment on lines
14
to
15
|
||||||||||
| # timeout_seconds is accepted for compatibility but ignored - PostgreSQL doesn't support | |
| # native timeouts with pg_try_advisory_lock, requiring Ruby-level polling instead | |
| # timeout_seconds is accepted for compatibility but ignored for PostgreSQL – neither | |
| # blocking nor non-blocking advisory locks support native timeouts; use Ruby-level polling instead |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code references PG::TRDeadlockDetected constant without checking if it's defined. If the PG gem is not loaded or a different PostgreSQL adapter is used, this will raise a NameError. Looking at line 48, PG::ConnectionBad is used similarly, suggesting this pattern already exists in the codebase. However, it would be safer to use the same defensive pattern as used with Mysql2 and Trilogy constants (see mysql_advisory.rb lines 47-50), checking if the constant is defined first, or wrapping the is_a? check to handle potential NameError.
| if blocking && (e.cause.is_a?(PG::TRDeadlockDetected) || e.message.include?('deadlock detected')) | |
| if blocking && ((defined?(PG::TRDeadlockDetected) && e.cause.is_a?(PG::TRDeadlockDetected)) || e.message.include?('deadlock detected')) |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,181 @@ | ||||||||
| # frozen_string_literal: true | ||||||||
|
|
||||||||
| require 'test_helper' | ||||||||
|
||||||||
| require 'test_helper' | |
| require 'test_helper' | |
| require 'concurrent' |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread synchronization for lock_acquired and thread1_finished uses plain boolean variables shared across threads without proper synchronization primitives (like Mutex or Concurrent::AtomicBoolean). While this may work in practice due to the sleep delays, it's not guaranteed to be thread-safe according to Ruby's memory model. The existing thread_test.rb (lines 13-28) uses Mutex.synchronize to protect shared state. Consider using Mutex or Concurrent::AtomicBoolean for thread-safe coordination, similar to how the deadlock test properly uses Concurrent::AtomicBoolean.
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deadlock detection test has a potential timing issue. The test expects exactly one of the two threads to detect a deadlock, but both threads have rescue blocks that set deadlock_detected = true (lines 78, 83, and 102). If both threads encounter the deadlock exception, the second assignment will overwrite the first, which is fine. However, if the deadlock is only detected in thread2 and not caught properly in thread1 (e.g., if thread1's block at line 75 returns false instead of raising), the test could still pass even though the behavior may not be as expected. Consider tracking which thread detected the deadlock for more robust testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When blocking is true, the code skips Ruby-level polling (line 65), but there's no validation to prevent users from combining blocking: true with timeout_seconds. For PostgreSQL, blocking locks wait indefinitely, so if a user specifies both
blocking: trueandtimeout_seconds: 5, the timeout will be silently ignored. Consider adding validation or documentation to clarify this behavior.