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 lock modes for cockroachdb #8250

Merged
merged 1 commit into from Nov 18, 2021
Merged

Conversation

wodka
Copy link
Contributor

@wodka wodka commented Oct 6, 2021

fixes #8249

Description of change

allow the same lock modes as postgres - https://www.cockroachlabs.com/docs/stable/select-for-update.html

currently an exception is thrown when a lock is used in a transaction

Adds Support for:

  • pessimistic_write
  • pessimistic_write_or_fail
  • for_no_key_update

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@wodka wodka force-pushed the cockroach-lock branch 9 times, most recently from 2d79b7a to bb87630 Compare October 7, 2021 10:21
@wodka
Copy link
Contributor Author

wodka commented Oct 7, 2021

:/ at this point I have no clue why the test are failing - there should be based on the code not be a change in postgre itself and cockroach compains about a part that should also not be affected.

@imnotjames
Copy link
Contributor

I've kicked off the tests again. I think the non-cockroach failures were just flakey tests that we should eventually remove.

The cockroach failures seemed to point at some sort of an issue.

@imnotjames imnotjames added the hacktoberfest-accepted label hacktoberfest label Oct 10, 2021
@wodka
Copy link
Contributor Author

wodka commented Oct 11, 2021

@imnotjames the previous test fails with a non recoverable error in the connection - and then cannot recover before the next test
image

@pleerock
Copy link
Member

can you check if version running in CI match the version where this feature support was added?

@wodka
Copy link
Contributor Author

wodka commented Oct 23, 2021

@pleerock I'm using CockroachDB (ver. 21.1.10) locally - and in circleCI cockroachdb/cockroach:latest-v21.1 is beeing run (21.1.10)

only difference is that locally I am using node 16 and ci runs with node 12

@pleerock
Copy link
Member

I've restarted CockroachDB tests again, and if they fail... You need to figure out why, because I don't have ideas. Maybe you can try to remove your code and do pushes one by one until it fail?

@wodka
Copy link
Contributor Author

wodka commented Nov 17, 2021

I'll try that - hopefully it will yield some result^^

@pleerock
Copy link
Member

Still failing.

@wodka
Copy link
Contributor Author

wodka commented Nov 18, 2021

pushed a new change - seems like that at one place when using the exception the transaction is not closed.

it("should throw error when specifying a table that is not part of the query", () => Promise.all(connections.map(async connection => {
        if (!(connection.driver instanceof PostgresDriver || connection.driver instanceof CockroachDriver))
            return;

        return connection.manager.transaction(entityManager => {
            return Promise.all([
                entityManager.createQueryBuilder(Post, "post")
                    .innerJoin("post.author", "user")
                    .setLock("pessimistic_write", undefined, ["img"])
                    .getOne()
                    // having the should.be here will prevent the transaction from closing.
                    //.should.be.rejectedWith('relation "img" in FOR UPDATE clause not found in FROM clause'),
            ]);
         // moving the should.be here will allow the transaction to be closed correctly.
        }).should.be.rejectedWith('relation "img" in FOR UPDATE clause not found in FROM clause');
    })));

:/ not sure why this behaviour happens in the first place as other exceptions do not break like this. I'm curious if this will now run through in circle ci

@wodka
Copy link
Contributor Author

wodka commented Nov 18, 2021

Woho :)

@pleerock pleerock merged commit d494fcc into typeorm:master Nov 18, 2021
@pleerock
Copy link
Member

Thank you!

HeartPattern pushed a commit to HeartPattern/typeorm that referenced this pull request Nov 29, 2021
HeartPattern pushed a commit to HeartPattern/typeorm that referenced this pull request Nov 29, 2021
@wodka wodka deleted the cockroach-lock branch July 6, 2022 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted label hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cockroach Lock Modes
3 participants