-
Notifications
You must be signed in to change notification settings - Fork 152
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
Use skip locked #303
Use skip locked #303
Conversation
👀 |
@JasonHerr, thanks for this - it looks good, though I think I want to look to make it backwards compatible. If you're interested, let me know, if not I may do it. |
@ukd1 I would not be able to do it any time soon. However, I could do it when I get some free time. My questions would be:
I guess I'm basically asking if you're suggesting it be backwards compatible with older PostgreSQL or just backwards compatible with QC? Thanks, |
@JasonHerr - tldr, backwards compatible with QC. I've decided that the next version of QC will be only for Ruby >= 2.4, and PG > 9.6 only, as a) the older versions are still around b) these are currently also pretty old. However, I'd like to not break installs if possible - so I think for now adding |
@JasonHerr Thank you so much for taking this and implementing it! Really appreciate it. One thing I noticed when looking through the PR is that since the new query is just a single SQL statement there's no need to have a |
@MSch actually, great point - this would also make it backwards compatible. |
Yes, I missed that. I can just modify queue.rb and add the select statement there AND leave top_bound alone. |
0c6f994
to
b96ef4c
Compare
Got time to look at this today. This was much simpler. I haven't run it on one of my performance boxes yet but, the concept remains the same. |
@JasonHerr nice, looking! |
Seems good to me; benchmarked it using the queue-shootout project from #279 (also, forked for some updates I'll do --> qc org). tldr, it's comparable to faster than que with these changes. all the tests pass; I can't see anything wrong / not backwards compatible. master:
full output -- master (59b3570) :
b96ef4c :
|
@shosti could I get a once over on this just for sanity? |
I'm going merge this - but via #311 as I've merged master, run the tests on circle and updated the changelog. Thx @JasonHerr! |
Glad to see it in! Thanks! |
This is so rad! |
I just saw these benchmarks as well - sick! |
This was inspired by work done by @MSch and documented in #279. I realize you're not looking to go to PostgreSQL 9.5 (or later) quite yet, but I needed this speed up. For large numbers of very small jobs and eight plus workers, this was a massive improvement. I've tested performance in my environment and I think your mileage may vary based on your job's computational size, frequency, number of workers, and so on.
Thanks so much to @MSch for doing his comparisons and proof of concept work.
I would bump the versioning but am loathe to do so until someone thinks this is likely to make it into master. I'll be referencing a git tag in my Gemfile until then.