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

Support SELECT .. FOR NO KEY UPDATE / KEY SHARE row level locking clauses in Postgres #4755

Merged
merged 2 commits into from Oct 23, 2021

Conversation

domkck
Copy link
Contributor

@domkck domkck commented Oct 20, 2021

In addition to SELECT .. FOR UPDATE and SELECT .. FOR SHARE, Postgres supports FOR NO KEY UPDATE and FOR KEY SHARE row level locking clauses https://www.postgresql.org/docs/14/explicit-locking.html#LOCKING-ROWS.

This adds support as .forNoKeyUpdate() and forKeyShare().

I don't know if there's an equivalent feature in MySQL and MSSQL - how should that be handled?

Fixes #4754

@@ -1773,6 +1774,77 @@ describe('Selects', function () {
});
});

it('select for no key update doesnt stop other transactions from inserting into tables that have a foreign key relationship', async function () {
if (!isPostgreSQL(knex)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it doesn't work with cockroach or redshift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like cockroach doesn't have support for these yet. As far as I can tell from this issue cockroachdb/cockroach#52420, it supports the keywords, but they are aliases for FOR UPDATE/FOR SHARE, which would make these tests fail.

I don't think Redshift has support for row level locks at all? https://docs.aws.amazon.com/redshift/latest/dg/r_UPDATE.html

But please let me know if you'd like these handled differently in the tests.

@kibertoad
Copy link
Collaborator

Looks good! Could you please also submit documentation PR to https://github.com/knex/documentation?

@domkck
Copy link
Contributor Author

domkck commented Oct 21, 2021

Awesome! Thank you.

Documentation PR here knex/documentation#346

@kibertoad kibertoad merged commit a17cc32 into knex:master Oct 23, 2021
@kibertoad
Copy link
Collaborator

Thank you!

@ekzyis
Copy link

ekzyis commented Oct 25, 2021

Hey, thanks for you work @domkck! Funny to see that a feature that I am in dire need now was implemented just two days ago :D

@kibertoad, can you give an estimate when this will get released?

For now, I installed that specific commit using npm install git://github.com/knex/knex#a17cc3214aadc5fec7f0c3ae85bc22d61e1462ed

@kibertoad
Copy link
Collaborator

@ekzyis I expect new RC by the end of this week, and final release next week, but it might actually be sooner.

@kibertoad
Copy link
Collaborator

@ekzyis This is now available in 0.95.12-rc5. Would you consider testing it and reporting back?

@ekzyis
Copy link

ekzyis commented Oct 25, 2021

Hey, thanks for your fast responses, I really appreciate it!

6 hours ago, I thought I need this feature to fix some query timeouts. I thought some transactions are unnecessary blocking each other but I was wrong. I ran a17cc32 but my issue still occurred so I looked deeper into it and found out that the transactions are not blocking each other but my connection pool was exhausted.

So sorry if I made you any trouble 🙈 Since my scenario no longer needs that feature, I also don't think I can test it as you hoped for. All I can say is that I ran a17cc32 and everything still worked fine in our dev stage if that helps 😄

But I looked over the commit diff and left a comment: a17cc32#r58638860

@domkck
Copy link
Contributor Author

domkck commented Oct 26, 2021

Not tried the rc directly, but we've been running 2ed68e9 installed from my fork in production for the last few days without any issues.

OlivierCavadenti pushed a commit to AbeonaPascha/knex that referenced this pull request Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SELECT .. FOR NO KEY UPDATE locking clause in Postgres
3 participants