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.

Sign up for GitHub

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

Jump to bottom

fix: Deadlock in PostgreSQL. #4445

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

fix: Deadlock in PostgreSQL. #4445

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9ef84c3
fix: Deadlock in PostgreSQL.
armory-abedonik Apr 6, 2023
c0788cb
feat: add comment.
armory-abedonik Apr 12, 2023
a81744e
feat: replace multiple updates requests with a single requests.
armory-abedonik Apr 14, 2023
2b6e899
feat: Add comment.
armory-abedonik Apr 14, 2023
dc3ea00
fix: tests.
armory-abedonik Apr 17, 2023
0d66eef
Merge branch 'master' into BOB-31035
armory-abedonik Apr 17, 2023
d5dab60
Merge branch 'master' into BOB-31035
jasonmcintosh Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions keiko-sql/src/main/kotlin/com/netflix/spinnaker/q/sql/SqlQueue.kt
View file
Open in desktop
Expand Up @@ -269,7 +269,7 @@ class SqlQueue(
* [MaxAttemptsAttribute] has been set to a positive integer. Otherwise,
* [AttemptsAttribute] is unused.
*/
val candidates = jooq.select(idField)
var candidates = jooq.select(idField)
.from(queueTable)
.where(deliveryField.le(now), lockedField.eq("0"))
.orderBy(deliveryField.asc())
Expand All @@ -281,7 +281,7 @@ class SqlQueue(
return
}

candidates.shuffle()
Copy link
Member

Choose a reason for hiding this comment

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

SO MOSTLY concerned b/c if I'm reading the comments ABOVE correct:

 To minimize lock contention, this is a non-locking read. The id's returned may be
 locked or removed by another instance before we can acquire them. We read more id's
 than [maxMessages] and shuffle them to decrease the likelihood that multiple instances
 polling concurrently are all competing for the oldest ready messages when many more
 than [maxMessages] are read.

The shuffle shouldn't have mattered. Except... it turns out it DOES matter b/c of lock behavior. Wondering if we can update the comments or explanations on this... OR refactor this code to be less... confusing on how it operates in combination with below which does seem to use a lock mechanism...

candidates = candidates.sorted()
armory-abedonik marked this conversation as resolved.
Show resolved Hide resolved

var position = 0
var passes = 0
Expand Down