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

Ensure uniqueness across all args if no keys are specified #1031

Merged

Conversation

sannek
Copy link
Contributor

@sannek sannek commented Feb 7, 2024

Hello!

Today I spent a good chunk of time chasing down some weirdness with certain jobs not getting scheduled, and I'm pretty sure this is a bug 🐛 . In certain cases, the order of job insertion affects whether or not uniqueness is enforced. (I noticed this with Oban.Pro.Engine.Smart , and that seems to use the same function for determining uniqueness, so if this is an acceptable fix it would be great if it can be fixed there too!)

Given a worker that has unique: [fields: [:args]] specified the following happens: inserting A first, and then B results in two separate jobs, as expected. But if you insert B first, and then insert A, it is considered to not be unique compared to B.

a = MiniUniq.new(%{id: 1})
b = MiniUniq.new(%{id: 1, extra: :cool_beans})

Initially I just wanted to open an issue, but I got curious and figured I might as well try to fix it. There is no Postgres operator to determine if two jsonb fields are exactly equal, so the best way seems to be to use both the @> and <@ operators to ensure that they are completely the same. I'm not very familiar with SQLite, so I extrapolated the same principle into that engine, but there might be a better way.

I added a test that initially confirmed the unexpected behaviour, and now confirms that it is consistent.

Lastly - locally mix format --check-formatted failed, but it is only upset by files that have not been changed in this PR, so I'm not sure what the best approach is there.

@sorentwo sorentwo changed the title Fix: ensure uniqueness across all args if no keys are specified Ensure uniqueness across all args if no keys are specified Feb 7, 2024
@sorentwo sorentwo added kind:bug Something isn't working area:oss Related to Oban OSS labels Feb 7, 2024
@sorentwo
Copy link
Member

sorentwo commented Feb 7, 2024

Thank you for taking the time to track this down and not only report it, but submit a PR! I've run the updated query in the web demo's database to check the query plan and timing, and it's just as quick as a single containment check (note the data is fake, that's not a real email):

-- Naive with =, unoptimized

 Gather  (cost=1000.00..76844.94 rows=1 width=8) (actual time=347.650..349.367 rows=1 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   ->  Parallel Seq Scan on oban_jobs  (cost=0.00..75844.84 rows=1 width=8) (actual time=274.450..338.798 rows=0 loops=3)
         Filter: (args = '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
         Rows Removed by Filter: 269338
 Planning Time: 0.106 ms
 Execution Time: 349.399 ms
(8 rows)

-- Original, using a single @>

 Bitmap Heap Scan on oban_jobs  (cost=182.13..271.97 rows=81 width=8) (actual time=2.233..2.234 rows=1 loops=1)
   Recheck Cond: (args @> '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
   Heap Blocks: exact=1
   ->  Bitmap Index Scan on oban_jobs_args_index  (cost=0.00..182.11 rows=81 width=0) (actual time=2.221..2.221 rows=1 loops=1)
         Index Cond: (args @> '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
 Planning Time: 0.124 ms
 Execution Time: 2.262 ms

-- Accurate, using both @> and <@

 Bitmap Heap Scan on oban_jobs  (cost=163.41..253.45 rows=1 width=8) (actual time=2.000..2.001 rows=1 loops=1)
   Recheck Cond: (args @> '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
   Filter: (args <@ '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
   Heap Blocks: exact=1
   ->  Bitmap Index Scan on oban_jobs_args_index  (cost=0.00..163.41 rows=81 width=0) (actual time=1.989..1.989 rows=1 loops=1)
         Index Cond: (args @> '{"email": "[email protected]", "project": "web-enabled"}'::jsonb)
 Planning Time: 0.135 ms
 Execution Time: 2.023 ms

Finally, I pushed a commit that addressed a module redefinition warning, made cond consistent with others in the codebase, and only encodes the args once in the lite engine.

@sorentwo sorentwo merged commit 8a2470d into oban-bg:main Feb 7, 2024
2 checks passed
@sannek sannek deleted the sanne/ensure-uniqueness-across-all-args branch February 7, 2024 20:06
@sannek
Copy link
Contributor Author

sannek commented Feb 7, 2024

Thank you for fixing up my PR and reviewing and merging it so quickly! You can probably imagine it was a bit of a headscratcher 😅

@sorentwo
Copy link
Member

sorentwo commented Feb 7, 2024

Thank you for fixing up my PR and reviewing and merging it so quickly!

No, thank you 🙂

These fixes are also ported to Pro for the next patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:oss Related to Oban OSS kind:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants