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

[8.x] Add support for MariaDB to skip locked rows with the database queue driver #39311

Merged
merged 2 commits into from
Oct 22, 2021
Merged

[8.x] Add support for MariaDB to skip locked rows with the database queue driver #39311

merged 2 commits into from
Oct 22, 2021

Conversation

antoniopaisfernandes
Copy link
Contributor

As documented in https://mariadb.com/kb/en/select/#skip-locked since MariaDB 10.6 there is support for skipping locked rows when fetching from the database.

This change adds support to it.

antoniopaisfernandes and others added 2 commits October 22, 2021 12:49
…river

As documented in https://mariadb.com/kb/en/select/#skip-locked since MariaDB 10.6 there is support for skipping locked rows when fetching from the database.

This change adds support to it.
@taylorotwell taylorotwell merged commit f758b16 into laravel:8.x Oct 22, 2021
@RensBruil
Copy link

This update actually crashed our queue workers. We updated the framework/laravel version from v8.64.0 to v8.67.0 and suddenly all of our queue workers conflicted because of the FOR UPDATE SKIP LOCKED. Our MariaDB version is: 10.3.31-MariaDB-log - MariaDB Server. Rolling back to v8.64.0 solved the issue.

@antoniopaisfernandes
Copy link
Contributor Author

Can you please show your result for:

$databaseEngine = \DB::getPdo()->getAttribute(PDO::ATTR_DRIVER_NAME);
$databaseVersion = \DB::getPdo()->getAttribute(PDO::ATTR_SERVER_VERSION);

echo version_compare($databaseVersion, '8.0.1', '>='));
echo strpos($databaseVersion, 'MariaDB');
echo version_compare(\Str::after($databaseVersion, '-'), '10.6.0', '>=');

Thanks

@RensBruil
Copy link

It just returns 18 if I run your script. The 3 echo's are:
version_compare($databaseVersion, '8.0.1', '>=') = true;
strpos($databaseVersion, 'MariaDB'); = 8
version_compare(\Str::after($databaseVersion, '-'), '10.6.0', '>='); = false

@xavi7th
Copy link

xavi7th commented Oct 26, 2021

I can confirm this same issues. I have been debugging for the past 3 days. I only just reaslised that the issue started after I upgraded.

I am also on version 10.3.31-MariaDB-log-cll-lve - MariaDB Server and Laravel 8.67.

Kindly look into it

@antoniopaisfernandes
Copy link
Contributor Author

Will refactor the conditional.

image

@antoniopaisfernandes
Copy link
Contributor Author

if (($databaseEngine === 'mysql' && ! strpos($databaseVersion, 'MariaDB') && version_compare($databaseVersion, '8.0.1', '>=')) ||
    (strpos($databaseVersion, 'MariaDB') && version_compare(Str::after($databaseVersion, '5.5.5-'), '10.6.0', '>=')) ||
    ($databaseEngine === 'pgsql' && version_compare($databaseVersion, '9.5', '>='))) {
    return 'FOR UPDATE SKIP LOCKED';
}

@thotam
Copy link

thotam commented Oct 27, 2021

shed our queue workers. We updated the framework/laravel version from v8.64.0 to v8.67.0 and suddenly all of our queue workers conflicted because of the FOR UPDATE SKIP LOCKED. Our MariaDB version is: 10.3.31-MariaDB-log - MariaDB Server. Rolling back to v8.64.0 solved the issue.

Same me

@coziboy
Copy link

coziboy commented Oct 27, 2021

This update actually crashed our queue workers. We updated the framework/laravel version from v8.64.0 to v8.67.0 and suddenly all of our queue workers conflicted because of the FOR UPDATE SKIP LOCKED. Our MariaDB version is: 10.3.31-MariaDB-log - MariaDB Server. Rolling back to v8.64.0 solved the issue.

same here

@taylorotwell
Copy link
Member

Revert it all.

@taylorotwell
Copy link
Member

@antoniopaisfernandes please test your changes. I will be merging nothing else in this regard for a while.

@antoniopaisfernandes
Copy link
Contributor Author

@taylorotwell Sorry for the mess I put you in :(

I'll find out more scenarios and how to improve the code.

@driesvints
Copy link
Member

@antoniopaisfernandes think it's best that we leave this be for now.

@antoniopaisfernandes
Copy link
Contributor Author

Thank you and apologies.

@driesvints
Copy link
Member

I did a new attempt for this at #39396

@mxts
Copy link

mxts commented Oct 28, 2021

I just feel this is easier to read.. and change

        if (Str::of($databaseVersion)->contains('MariaDB')) {
            $databaseEngine = 'MariaDB';
            $databaseVersion = Str::of($databaseVersion)->match("/\d+\.\d+\.\d+/");
        }

        if (($databaseEngine === 'mysql' && version_compare($databaseVersion, '8.0.1', '>=')) ||
            ($databaseEngine === 'MariaDB' && version_compare($databaseVersion, '10.6.0', '>=')) ||
            ($databaseEngine === 'pgsql' && version_compare($databaseVersion, '9.5', '>='))) {
            $result = 'FOR UPDATE SKIP LOCKED';
        }

@driesvints
Copy link
Member

Thanks @mxts, I added that suggestion, that looks indeed better. The regex wasn't correct though as it won't strip the 5.5.5- part.

@driesvints
Copy link
Member

Btw I'd greatly appreciate it if you all could try out #39396 to see if it passes for your systems.

@antoniopaisfernandes @RensBruil @xavi7th @thotam @coziboy

@antoniopaisfernandes
Copy link
Contributor Author

@driesvints Thank you so much for your care! 🦸‍♂️

With vanilla MariaDB 10.5:

211028 18:34:58     20 Connect  root@localhost on testapp using TCP/IP
                    20 Query    use `testapp`
                    20 Prepare  set names 'utf8mb4' collate 'utf8mb4_unicode_ci'
                    20 Execute  set names 'utf8mb4' collate 'utf8mb4_unicode_ci'
                    20 Close stmt
                    20 Prepare  set session sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'
                    20 Execute  set session sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'
                    20 Close stmt
                    20 Query    START TRANSACTION
                    20 Prepare  select * from `jobs` where `queue` = ? and ((`reserved_at` is null and `available_at` <= ?) or (`reserved_at` <= ?)) order by `id` asc limit 1 for update
                    20 Execute  select * from `jobs` where `queue` = 'default' and ((`reserved_at` is null and `available_at` <= 1635446098) or (`reserved_at` <= 1635446008)) order by `id` asc limit 1 for update
                    20 Close stmt
                    20 Query    COMMIT

With 10.6:

Tcp port: 3306  Unix socket: /var/lib/mysql/mysql.sock
Time                Id Command  Argument
211028 18:38:47     21 Connect  root@localhost on testapp using TCP/IP
                    21 Query    use `testapp`
                    21 Prepare  set names 'utf8mb4' collate 'utf8mb4_unicode_ci'
                    21 Execute  set names 'utf8mb4' collate 'utf8mb4_unicode_ci'
                    21 Close stmt
                    21 Prepare  set session sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'
                    21 Execute  set session sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'
                    21 Close stmt
                    21 Query    START TRANSACTION
                    21 Prepare  select * from `jobs` where `queue` = ? and ((`reserved_at` is null and `available_at` <= ?) or (`reserved_at` <= ?)) order by `id` asc limit 1 FOR UPDATE SKIP LOCKED
                    21 Execute  select * from `jobs` where `queue` = 'default' and ((`reserved_at` is null and `available_at` <= 1635446327) or (`reserved_at` <= 1635446237)) order by `id` asc limit 1 FOR UPDATE SKIP LOCKED
                    21 Close stmt
                    21 Query    COMMIT

No errors running the workers! 🥳

@mxts
Copy link

mxts commented Oct 29, 2021

@antoniopaisfernandes

Digress a bit here, how did you all manage to find the issue?

  • I only looked into it because emails were not being dispatched.
  • Running php artisan queue:work did not produce any exceptions and did not run any jobs
  • It took me quite a bit of debugging because I wasn't closely monitoring my development honeybadger.

What I mean is, is there a "best" way to look out for/catch such exceptions?

@antoniopaisfernandes
Copy link
Contributor Author

@mxts For the test I just activated MariaDB logging SET GLOBAL general_log=1 and started a worker with the Laravel framework at the commit @driesvints made.

As for being aware of problems in the runners, our choice was Sentry https://sentry.io/for/laravel/ but there are a lot of solutions for it.

@bilogic
Copy link
Contributor

bilogic commented Oct 30, 2021

@antoniopaisfernandes thanks I was hoping for something to appear when I run queue:work

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.

9 participants