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

ricmzn · 2018-12-18T15:17:12Z

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

kibertoad · 2018-12-18T15:22:43Z

src/query/builder.js

+  // 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)) {


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

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.

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

Edit: sorry, the commit id was wrong

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

test/integration/builder/selects.js

kibertoad · 2018-12-18T15:25:26Z

@kukiric Try mysql2 driver, it should support MySQL 8.0

ricmzn · 2018-12-19T15:32:57Z

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 · 2019-01-07T17:53:13Z

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 · 2019-01-07T17:55:01Z

@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 · 2019-01-08T13:53:38Z

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

test/integration/builder/selects.js

kibertoad · 2019-03-03T17:27:30Z

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

ricmzn · 2019-03-06T17:12:02Z

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 · 2019-03-06T17:25:07Z

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

kibertoad

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

ricmzn · 2019-03-12T20:31:19Z

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 · 2019-03-12T20:37:12Z

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

ricmzn · 2019-03-13T18:32:57Z

Documentation PR is up at knex/documentation#186

elhigu

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

kibertoad · 2019-06-13T22:11:23Z

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

ricmzn · 2019-06-14T23:28:16Z

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 · 2019-06-18T23:43:44Z

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

(For real this time, without squashing into a mess of merge conflicts)

ricmzn · 2019-06-21T12:52:54Z

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)

test/integration/builder/selects.js

kibertoad · 2019-06-21T21:47:07Z

src/query/builder.js

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


probably these could be constants?

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?

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.

Ok, I'll try

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

kibertoad · 2019-06-26T14:08:01Z

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

kibertoad · 2019-07-06T12:05:47Z

Thanks a lot!

kibertoad · 2019-07-10T20:53:48Z

Feature is available in 0.18.4

kibertoad · 2019-08-14T20:28:17Z

@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 · 2019-08-15T16:30:47Z

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

kibertoad · 2019-08-15T16:34:59Z

@kukiric Much appreciated!

ricmzn · 2019-08-16T01:25:11Z

@kibertoad PR submitted: #3398

Implement "skip locked" and "nowait" for Postgres dialect

150fb08

kibertoad reviewed Dec 18, 2018

View reviewed changes

src/query/builder.js Outdated Show resolved Hide resolved

kibertoad reviewed Dec 18, 2018

View reviewed changes

test/integration/builder/selects.js Outdated Show resolved Hide resolved

ricmzn added 2 commits December 19, 2018 12:42

Fix error with duplicate skipLocked or noWait call

b9dd2c6

Fix noWait test swallowing errors on non-postgres databases

21775f9

Implement skipLocked and noWait for mysql-based clients

0f13016

kibertoad added the missing documentation label Jan 7, 2019

ricmzn added 3 commits January 8, 2019 10:36

Disable skipLocked test on MySQL

d73663a

Restrict noWait test to supported databases

dbac68e

Update knex.d.ts with skipLocked and noWait

71fd14b

ricmzn added 2 commits January 8, 2019 13:10

Adjust noWait test for MySQL 8.0

de3b4cc

Disable noWait test on MySQL

94de977

kibertoad reviewed Jan 8, 2019

View reviewed changes

test/integration/builder/selects.js Outdated Show resolved Hide resolved

Increase query builder test coverage for MySQL

7db84fa

ricmzn mentioned this pull request Jan 9, 2019

Upgrade MySQL in Travis to 8.0 #2988

Closed

Remove MySQL support for skipLocked() and noWait()

e49c4b7

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 reviewed Mar 12, 2019

View reviewed changes

Add errors for databases that don't support skipLocked() and noWait()

f32488f

kibertoad reviewed Mar 12, 2019

View reviewed changes

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

elhigu requested changes Mar 13, 2019

View reviewed changes

kibertoad added missing tests and removed missing documentation labels Mar 13, 2019

ricmzn added 3 commits June 20, 2019 00:52

Merge remote-tracking branch 'upstream/master' into feature/skip-locked

6b44e37

(For real this time, without squashing into a mess of merge conflicts)

Update test comments

dcaff9e

Restore MySQL tests for #2961

18cccab

ricmzn added 3 commits June 21, 2019 10:31

Simplify query builder checks on first(), noWait() and skipLocked()

e9b152d

Add some failing tests for PR #2961

2a92c9d

Replace broken test on MySQL with an extra test for #2961

6eb290a

kibertoad reviewed Jun 21, 2019

View reviewed changes

test/integration/builder/selects.js Show resolved Hide resolved

kibertoad reviewed Jun 21, 2019

View reviewed changes

test/integration/builder/selects.js Outdated Show resolved Hide resolved

kibertoad reviewed Jun 21, 2019

View reviewed changes

kibertoad approved these changes Jun 26, 2019

View reviewed changes

ricmzn added 2 commits June 29, 2019 19:22

Adjust the #2961 tests to use async/await

b22cd7b

Move some magic query builder values into constants

c0b1cd4

kibertoad merged commit bc1ddca into knex:master Jul 6, 2019

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

Implement "skipLocked()" and "noWait()" (knex#2961)

19e1316

ricmzn mentioned this pull request Aug 16, 2019

Fix broken noWait test #3398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ricmzn commented Dec 18, 2018 •

edited

kibertoad Dec 18, 2018

ricmzn Dec 19, 2018

ricmzn Jun 21, 2019 •

edited

ricmzn Jun 21, 2019 •

edited

kibertoad commented Dec 18, 2018

ricmzn commented Dec 19, 2018 •

edited

ricmzn commented Jan 7, 2019

kibertoad commented Jan 7, 2019

ricmzn commented Jan 8, 2019

kibertoad commented Mar 3, 2019

ricmzn commented Mar 6, 2019 •

edited

kibertoad commented Mar 6, 2019

kibertoad left a comment

ricmzn commented Mar 12, 2019 •

edited

ricmzn commented Mar 12, 2019

ricmzn commented Mar 13, 2019 •

edited

elhigu left a comment

kibertoad commented Jun 13, 2019

ricmzn commented Jun 14, 2019 •

edited

kibertoad commented Jun 18, 2019

ricmzn commented Jun 21, 2019 •

edited

kibertoad Jun 21, 2019

ricmzn Jun 28, 2019

kibertoad Jun 28, 2019

ricmzn Jun 29, 2019

ricmzn Jul 3, 2019

kibertoad commented Jun 26, 2019

kibertoad commented Jul 6, 2019

kibertoad commented Jul 10, 2019

kibertoad commented Aug 14, 2019 •

edited

ricmzn commented Aug 15, 2019

kibertoad commented Aug 15, 2019

ricmzn commented Aug 16, 2019

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

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

Conversation

ricmzn commented Dec 18, 2018 • edited

kibertoad Dec 18, 2018

Choose a reason for hiding this comment

ricmzn Dec 19, 2018

Choose a reason for hiding this comment

ricmzn Jun 21, 2019 • edited

Choose a reason for hiding this comment

ricmzn Jun 21, 2019 • edited

Choose a reason for hiding this comment

kibertoad commented Dec 18, 2018

ricmzn commented Dec 19, 2018 • edited

ricmzn commented Jan 7, 2019

kibertoad commented Jan 7, 2019

ricmzn commented Jan 8, 2019

kibertoad commented Mar 3, 2019

ricmzn commented Mar 6, 2019 • edited

kibertoad commented Mar 6, 2019

kibertoad left a comment

Choose a reason for hiding this comment

ricmzn commented Mar 12, 2019 • edited

ricmzn commented Mar 12, 2019

ricmzn commented Mar 13, 2019 • edited

elhigu left a comment

Choose a reason for hiding this comment

kibertoad commented Jun 13, 2019

ricmzn commented Jun 14, 2019 • edited

kibertoad commented Jun 18, 2019

ricmzn commented Jun 21, 2019 • edited

kibertoad Jun 21, 2019

Choose a reason for hiding this comment

ricmzn Jun 28, 2019

Choose a reason for hiding this comment

kibertoad Jun 28, 2019

Choose a reason for hiding this comment

ricmzn Jun 29, 2019

Choose a reason for hiding this comment

ricmzn Jul 3, 2019

Choose a reason for hiding this comment

kibertoad commented Jun 26, 2019

kibertoad commented Jul 6, 2019

kibertoad commented Jul 10, 2019

kibertoad commented Aug 14, 2019 • edited

ricmzn commented Aug 15, 2019

kibertoad commented Aug 15, 2019

ricmzn commented Aug 16, 2019

ricmzn commented Dec 18, 2018 •

edited

ricmzn Jun 21, 2019 •

edited

ricmzn Jun 21, 2019 •

edited

ricmzn commented Dec 19, 2018 •

edited

ricmzn commented Mar 6, 2019 •

edited

ricmzn commented Mar 12, 2019 •

edited

ricmzn commented Mar 13, 2019 •

edited

ricmzn commented Jun 14, 2019 •

edited

ricmzn commented Jun 21, 2019 •

edited

kibertoad commented Aug 14, 2019 •

edited