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
Merged

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

merged 20 commits into from Jul 6, 2019

Conversation

ricmzn
Copy link
Contributor

@ricmzn ricmzn 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

src/query/builder.js Outdated Show resolved Hide resolved
// 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)) {
Copy link
Collaborator

@kibertoad kibertoad Dec 18, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ricmzn ricmzn Dec 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ricmzn ricmzn Jun 21, 2019

Choose a reason for hiding this comment

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

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

Edit: sorry, the commit id was wrong

Copy link
Contributor Author

@ricmzn ricmzn Jun 21, 2019

Choose a reason for hiding this comment

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

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Dec 18, 2018

@kukiric Try mysql2 driver, it should support MySQL 8.0

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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?

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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
Copy link
Collaborator

@kibertoad kibertoad 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.

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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
Copy link
Collaborator

@kibertoad kibertoad commented Mar 3, 2019

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

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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
Copy link
Collaborator

@kibertoad kibertoad commented Mar 6, 2019

Awesome!

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
Copy link
Collaborator

@kibertoad kibertoad left a comment

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

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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.

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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.

src/query/compiler.js Outdated Show resolved Hide resolved
@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn commented Mar 13, 2019

Documentation PR is up at knex/documentation#186

Copy link
Member

@elhigu elhigu 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
Copy link
Collaborator

@kibertoad kibertoad commented Jun 13, 2019

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

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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
Copy link
Collaborator

@kibertoad kibertoad commented Jun 18, 2019

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

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn 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') {
Copy link
Collaborator

@kibertoad kibertoad Jun 21, 2019

Choose a reason for hiding this comment

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

probably these could be constants?

Copy link
Contributor Author

@ricmzn ricmzn Jun 28, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@kibertoad kibertoad Jun 28, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ricmzn ricmzn Jun 29, 2019

Choose a reason for hiding this comment

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

Ok, I'll try

Copy link
Contributor Author

@ricmzn ricmzn Jul 3, 2019

Choose a reason for hiding this comment

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

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jun 26, 2019

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

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 6, 2019

Thanks a lot!

@kibertoad kibertoad merged commit bc1ddca into knex:master Jul 6, 2019
1 of 2 checks passed
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jul 10, 2019

Feature is available in 0.18.4

felixmosh pushed a commit to felixmosh/knex that referenced this issue Jul 13, 2019
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Aug 14, 2019

@kukiric Apparently there is a problem with tests,

      promise.then(() => {
        expect(
          'The query should have been cancelled when trying to select a locked row with .noWait()'
        ).to.be.false;

Is never returned or awaited, so promise ends up being an unhandled rejection behind the scenes.

Any chance you could fix either the test or the implementation, whichever is needed to make sure it passes? (simple awaiting is not enough, promise does end up reaching unexpected state)

You can see error here: https://travis-ci.org/tgriesser/knex/jobs/572000778

And if you want rejected promises to fail your tests, you can use #3393 as a base.

@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn commented Aug 15, 2019

Sure, give me a day to submit a PR to fix it.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Aug 15, 2019

@kukiric Much appreciated!

@ricmzn ricmzn mentioned this pull request Aug 16, 2019
@ricmzn
Copy link
Contributor Author

@ricmzn ricmzn commented Aug 16, 2019

@kibertoad PR submitted: #3398

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.

None yet

3 participants