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

WIP: Implement "skipLocked()" and "noWait()" #2961

Merged
merged 20 commits into from Jul 6, 2019

Conversation

Projects
None yet
3 participants
@kukiric
Copy link
Contributor

commented Dec 18, 2018

This PR aims to resolve issue #1937.

It adds two chainable methods into the query builder: skipLocked() and noWait(). They can only be used in a select-like query, and only after a lock mode has been specified with forShare() or forUpdate(). These methods simply append the requested lock mode onto the query, leaving the actual lock handling to the RDMS.

The main use case for these methods is to create a job queue-type system in the database, where each job can only be taken by a single process or thread. Without them, it's possible to create deadlocks with multiple consumers taking exclusive locks on the same rows, when skipping locked rows would otherwise be more ideal.

Currently, this only works with the PostgreSQL dialect. I wanted to add support for MySQL and MariaDB as well, but I can't get the test suite to connect to the latest version of those databases (the client drivers don't seem to support the new authentication mode added in MySQL 8.0), and older versions don't support these statements.

As a WIP Pull Request, I'm open to comments and suggestions. Help is also appreciated.

TODO:

  • PostgreSQL support
  • MySQL/MariaDB support (currently failing tests)
  • Update knex.d.ts
  • Add documentation
Show resolved Hide resolved src/query/builder.js Outdated
// Causes error when acessing a locked row instead of waiting for it to be released.
noWait() {
const { _method } = this;
if (!includes(['pluck', 'first', 'select'], _method)) {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Dec 18, 2018

Collaborator

any reasons why duplicate code can't be reused? do you envision it branching out eventually?

This comment has been minimized.

Copy link
@kukiric

kukiric Dec 19, 2018

Author Contributor

I'm actually not sure how to best handle this. I just copied this piece from the first() method, since all databases only allow "skip locked" and "nowait" on select queries. I could, however, just leave it up for the database driver to return the error directly to the calling code, without validating it first.

This comment has been minimized.

Copy link
@kukiric

kukiric Jun 21, 2019

Author Contributor

@kibertoad handled in commit e9b152d, what do you think?

Edit: sorry, the commit id was wrong

This comment has been minimized.

Copy link
@kukiric

kukiric Jun 21, 2019

Author Contributor

I'm also thinking about moving the "_isSelectQuery" checks to the "forShare" and "forUpdate" methods

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2018

@kukiric Try mysql2 driver, it should support MySQL 8.0

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

The mysql2 driver worked with MariaDB, but it seems something is wrong with my setup since I still get a "client does not support authentication protocol" error when trying to use either driver with the MySQL 8.0 docker image. MySQL 5.7 works, but it doesn't support these statements.

Also, I just tested this with MariaDB, and I've found out that it supports "no wait", but not "skip locked" like MySQL. How should the tests be handled in this scenario?

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Pinging @kibertoad

Should I drop MySQL + MariaDB support for this PR? The CI set up for this repo doesn't run a supported database version, and it's really inconvenient that MariaDB doesn't support this feature. PostgreSQL works like a charm, though.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jan 7, 2019

@kukiric Better to disable tests on these DBs temporary with ToDo to enable again when we use newer DB versions for testing. Extra bonus points if you could open an issue to add MySQL 8.0 to our test set.

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@kibertoad I'll open the issue later today. Currently, Travis doesn't support MySQL 8.0 directly, nor a recent enough Ubuntu version that can install it via apt. Also, both the mysql and mysql2 packages for node lack support for the new password algorithm that they added in 8.0.4. So for now, this is Postgres-only, with noWait() also incidentally working on newer versions of MariaDB.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2019

@kukiric Heya! Any chance this PR could be finished? (skipping all the incompatible DBs)

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Sorry, I got busy and forgot to finish the PR, mainly because I worked around it with a raw query at my company. I'll wrap it up with docs + error messages on other DB drivers this weekend.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

Awesome!

Remove MySQL support for skipLocked() and noWait()
Reasoning: while this feature is useful, it's untestable without the proper framework for testing on MySQL 8.0, and testing MySQL and MariaDB separately.

See: #2961
@kibertoad
Copy link
Collaborator

left a comment

Well, you could have kept the functionality and only skipped tests, no?

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

The removed functionality is just a pair of functions that return the strings to be appended to the SQL (in exactly the same format as Postgres), so it can be easily put back in when tests are in. It's fine by me if you want to re-enable it before merging, but I don't like to lead people into "here be dragons" territory when dealing with databases.

@kukiric kukiric force-pushed the kukiric:feature/skip-locked branch from ed18e90 to f32488f Mar 12, 2019

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I guess I'll leave that up to the documentation. I'm going to write the the first draft and make a PR for it as well.

Show resolved Hide resolved src/query/compiler.js Outdated
@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Documentation PR is up at knex/documentation#186

@elhigu
Copy link
Collaborator

left a comment

Most of the code paths throwing errors seem to be missing the tests. Those should be tested as well as the happy paths.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 13, 2019

@kukiric Would you consider finishing this, now that MySQL 8 is used in CI by default?

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Sure! I kinda forgot about it as I solved the original issue with a raw query and moved onto other things, but I can pick this up again.

Should I update the branch by merging or rebasing master, or leave it alone until it's done?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

@kukiric Either approach works, we squash PRs anyway, so whatever is more convenient for you.

Merge remote-tracking branch 'upstream/master' into feature/skip-locked
(For real this time, without squashing into a mess of merge conflicts)

@kukiric kukiric force-pushed the kukiric:feature/skip-locked branch from 74643ab to 6b44e37 Jun 20, 2019

@kukiric

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

MySQL support is back, but it seems to not be locking rows correctly with a limit clause on the latest (Docker-published) version, so the test will always fail.

Edit: https://stackoverflow.com/questions/5694658/how-many-rows-will-be-locked-by-select-order-by-xxx-limit-1-for-update - This is exactly what I'm getting here, so it looks like I'll have to re-write the test for MySQL (unless I'm using it incorrectly, which is also likely)

'.noWait() can only be used after a call to .forShare() or .forUpdate()!'
);
}
if (this._single.waitMode === 'skipLocked') {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jun 21, 2019

Collaborator

probably these could be constants?

This comment has been minimized.

Copy link
@kukiric

kukiric Jun 28, 2019

Author Contributor

I agree, but I can't find a good place for query builder constants. Is it worth it to create one now, or would this be more fitting for a future refactor?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jun 28, 2019

Collaborator

We don't get much refactoring PRs, as people have no good reason for submitting these :D. Would be awesome if you created something for this.

This comment has been minimized.

Copy link
@kukiric

kukiric Jun 29, 2019

Author Contributor

Ok, I'll try

This comment has been minimized.

Copy link
@kukiric

kukiric Jul 3, 2019

Author Contributor

Made it a separate module in c0b1cd4, how do you like it?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2019

I'm OK with merging it after code review comments are addressed.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2019

Thanks a lot!

@kibertoad kibertoad merged commit bc1ddca into tgriesser:master Jul 6, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.01%) to 88.081%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Feature is available in 0.18.4

felixmosh added a commit to felixmosh/knex that referenced this pull request Jul 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.