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

[9.x] Don't use locks for queue job popping for PlanetScale's MySQL-compatible Vitess engine #44027

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

rihardsgrislis
Copy link

@rihardsgrislis rihardsgrislis commented Sep 6, 2022

PlanetScale provides great serverless MySQL-compatible database and guide on how to integrate it in Laravel (https://planetscale.com/docs/tutorials/connect-laravel-app) but under hood uses Vitess.

However it looks that Vitess doesn't support "skip" queries:
SQLSTATE[HY000]: General error: 1105 syntax error at position 185 near 'SKIP' (SQL: select * from `jobs` where `queue` = default and ((`reserved_at` is null and `available_at` <= 1662479913) or (`reserved_at` <= 1662479823)) order by `id` asc limit 1 FOR UPDATE SKIP LOCKED)

Following #31536 I've added a check for Vitess engine/version parsing so it wouldn't use locks for popping:

>>> DB::connection()->getPdo()->getAttribute(PDO::ATTR_DRIVER_NAME)
=> "mysql"

>>> DB::connection()->getPdo()->getAttribute(PDO::ATTR_SERVER_VERSION)
=> "8.0.23-vitess"

PlanetScale provides great serverless MySQL-compatible database and guide on how to integrate it in Laravel (https://planetscale.com/docs/tutorials/connect-laravel-app) but under hood uses Vitess.

However it looks that Vitess doesn't support "skip" queries:
```SQLSTATE[HY000]: General error: 1105 syntax error at position 185 near 'SKIP' (SQL: select * from `jobs` where `queue` = default and ((`reserved_at` is null and `available_at` <= 1662479913) or (`reserved_at` <= 1662479823)) order by `id` asc limit 1 FOR UPDATE SKIP LOCKED)```

Following laravel#31536 I've improved engine/version parsing for Vitess so it wouldn't use locks for popping:
```>>> DB::connection()->getPdo()->getAttribute(PDO::ATTR_DRIVER_NAME)
=> "mysql"

>>> DB::connection()->getPdo()->getAttribute(PDO::ATTR_SERVER_VERSION)
=> "8.0.23-vitess"```
@rihardsgrislis rihardsgrislis changed the title Don't use locks for queue job popping for PlanetScale's MySQL-compatible Vitess engine [9.x] Don't use locks for queue job popping for PlanetScale's MySQL-compatible Vitess engine Sep 6, 2022
@taylorotwell
Copy link
Member

I wonder if we just let this continue to error though? If it doesn't support this feature then database queues are not really a great option?

@rihardsgrislis
Copy link
Author

rihardsgrislis commented Sep 6, 2022

@taylorotwell contacted PlanetScale support also and their response was:

Thank you Rihards for reaching out with your question!

For the FOR UPDATE SKIP LOCKED usage within the Laravel Queue system, it seems like this may be a limitation in our Vitess layer based on this open issue over here:
vitessio/vitess#5249

At the end of that issue it points to a pull request where some support may have been added in, but if you are experiencing that syntax error than more than likely it is not completely supported at the Vitess layer yet.

For a similar ticket related to Laravel Queues, it does seem that using Redis is generally the option that's most preferred, although the improvements for the database queue option, such as when the usage of FOR UPDATE SKIP LOCKED was added in to Laravel, has helped make the database option more viable. What is confusing for me at the moment however is that it did appear for another user that using the database queue option for Laravel appeared to be working for them, although perhaps they are using things in a slightly different way and don't have the same issue for some reason.

The team is aiming to bring up full parity support of MySQL 8 features to the Vitess layer over the coming year so this should be something that will be eventually resolved, and even though there is the existing Vitess issue shared above, I think it would be a good idea for me to submit an internal request about this as well, specifically to better support our Laravel users/community and hopefully get this resolved more quickly, so I'll be working on doing so as well, even though a solution there is not likely to be immediate.

In the meantime, I think the next best option though would be to use Redis if that is at all possible for your project.

I guess Redis is better choice now (which I already switched to) than maintaining this if they plan to resolve all compatibility issues soon anyway.
Unless there are more community members wanting to start their Laravel project with PlanetScale's database and this is a bottleneck? They have a great hobby plan for free which is why I choose them in the first place.

@taylorotwell taylorotwell merged commit fc9dc59 into laravel:9.x Sep 7, 2022
@taylorotwell
Copy link
Member

Will merge this for now but note that you may experience database deadlocks if you put a lot of queue workers in place and the database doesn't support skip locked, etc.

@BrandonSurowiec
Copy link
Contributor

Seems like a PR to add skip/lock support was just merged:
vitessio/vitess#13965

This PR may be able to be reverted soon.

@dihmeetree
Copy link

@BrandonSurowiec is this available in PlanetScale for you? I just tried but got the following error:

ERROR 1105 (HY000): syntax error at position 62 near 'SKIP'

@BrandonSurowiec
Copy link
Contributor

They haven't made a release since september: https://github.com/planetscale/vitess-releases/releases

@orware
Copy link

orware commented Dec 22, 2023

@rihardsgrislis, @BrandonSurowiec, @dihmeetree, and others subscribed to this issue.

This should likely work now within PlanetScale databases as most have been moved onto Vitess 18 which includes the support as part of our rollout for foreign key constraints support that arrived earlier this month.

@GrahamCampbell
Copy link
Member

They haven't made a release since september: https://github.com/planetscale/vitess-releases/releases

That is the wrong repo. The repo is https://github.com/vitessio/vitess/releases.

@crynobone
Copy link
Member

The PR was merged to main branch and is not yet released: vitessio/vitess@29a9bf4

CleanShot 2024-01-03 at 10 36 20

@GrahamCampbell
Copy link
Member

It looks like it'll be included in Vitess 19.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants