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

Allow Lockable to be passed custom column, key, and Postgres advisory lock/unlock function #273

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

bensheldon
Copy link
Owner

@bensheldon bensheldon commented Jun 22, 2021

@reczy
Copy link
Contributor

reczy commented Jun 23, 2021

Hey @bensheldon - thoughts on using a single multi-line string in the :advisory_lock scope rather than a series of arel commands? My opinion is that reading the sql in its entirety makes it a bit easier to grasp what's going on here.

@bensheldon
Copy link
Owner Author

@reczy I agree that using Arel has the regex problem of "now you have two problems". I have a few reasons to use Arel:

  • I'm personally leaning into Rails and ActiveRecord, which heavily involves Arel.
  • I'd like to extract the Lockable module into its own gem, and I'd like to retain some flexibility in what it can do and expect and decompose.

...that being written, I also don't want Arel to be a blocker for people contributing.

The sql query is documented in the tests:

it 'generates appropriate SQL' do
query = model_class.where(priority: 99).order(priority: :desc).limit(2).advisory_lock
expect(normalize_sql(query.to_sql)).to eq normalize_sql(<<~SQL.squish)
SELECT "good_jobs".*
FROM "good_jobs"
WHERE "good_jobs"."id" IN (
WITH "rows" AS #{'MATERIALIZED' if model_class.supports_cte_materialization_specifiers?} (
SELECT "good_jobs"."id"
FROM "good_jobs"
WHERE "good_jobs"."priority" = 99
ORDER BY "good_jobs"."priority" DESC
)
SELECT "rows"."id"
FROM "rows"
WHERE pg_try_advisory_lock(('x' || substr(md5('good_jobs' || "rows"."id"::text), 1, 16))::bit(64)::bigint)
LIMIT 2
)
ORDER BY "good_jobs"."priority" DESC
SQL
end

...but I can better document what's happening in the code itself too.

Also, thank you for commenting on this PR. I was a little premature in opening it (I gotta get better at "Draft" PRs)

@bensheldon bensheldon changed the title Allow Lockable to have custom lock key; use FOR UPDATE SKIP LOCKED Allow Lockable to be passed custom column, key, and Postgres advisory lock/unlock function Jun 24, 2021
@bensheldon
Copy link
Owner Author

bensheldon commented Jun 24, 2021

I extracted the FOR UPDATE SKIP LOCKED change to #274.

@bensheldon bensheldon added the refactor Code changes that do not introduce new features label Jun 24, 2021
@bensheldon bensheldon merged commit 129691c into main Jun 24, 2021
@bensheldon bensheldon deleted the lockable_key branch June 24, 2021 17:06
Backlog automation moved this from In progress to Done Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code changes that do not introduce new features
Projects
Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants