Enable skipping locked rows in QueryBuilder #6191

morozov · 2023-10-14T22:16:37Z

This is a continuation of the work started in #6105.

There is quite some code duplication between the SelectSQLBuilder implementations but it should be acceptable. An alternative would be to compose platform-specific builders from a set of smaller ones (e.g. for SELECT, FROM, FOR UPDATE, etc). It would reduce code duplication but might impact the performance: for simple queries, building the builder might take longer than building the actual query. If/when necessary, we can decompose the builders without breaking the existing API.

TODO:

Deprecate getReadLockSQL(), getWriteLockSQL() and getForUpdateSQL() as non-portable.
Add an integration test for forUpdate() without options. We cannot easily test the actual lock but we at least can test that the SQL syntax is valid across the platforms that provide the corresponding SQL snippets.
Make sure that the new API is extensible. We should be able to add the possibility build queries like FOR UPDATE OF <columns> or FOR UPDATE NO WAIT without breaking the API.
Mark new constructors as internal to avoid unnecessary BC breaks.

Future scope

We may want to improve the UX of the new API and instead of throwing an "unsupported" exception at the SQL build time, throw it during the call to the corresponding method (e.g. forUpdate()) on SQLite. For that, we may replace the portable SelectQuery value object with a set of platform-specific ones. This way a call from the query builder to the value object that doesn't support a certain feature would fail right away.

derrabus · 2023-11-01T21:54:49Z

tests/Functional/Query/QueryBuilderTest.php

+        if ($platform instanceof DB2Platform) {
+            self::markTestSkipped('Skipping on IBM DB2');
+        }
+
+        if ($platform instanceof MySQLPlatform) {
+            if ($platform instanceof MariaDBPlatform) {
+                if (! $platform instanceof MariaDb1060Platform) {
+                    self::markTestSkipped('Skipping on MariaDB older than 10.6');
+                }
+            } elseif (! $platform instanceof MySQL80Platform) {
+                self::markTestSkipped('Skipping on MySQL older than 8.0');
+            }
+        }
+
+        if ($platform instanceof PostgreSQLPlatform && ! $platform instanceof PostgreSQL100Platform) {
+            self::markTestSkipped('Skipping on PostgreSQL older than 10.0');
+        }
+
+        if ($platform instanceof SqlitePlatform) {
+            self::markTestSkipped('Skipping on SQLite');
+        }


Can we add a test that covers the behavior of SKIP_LOCKED for those platforms that don't support it?

derrabus · 2023-11-01T22:11:04Z

src/SQL/Builder/DefaultSelectSQLBuilder.php

+
+            if ($forUpdate->getConflictResolutionMode() === ConflictResolutionMode::SKIP_LOCKED) {
+                if ($this->skipLockedSQL === null) {
+                    throw Exception::notSupported('SKIP LOCKED');


Let's say I use the query builder to construct a query for an arbitrary database. Is there a way to find out that SKIP LOCKED is available? I probably don't want to run into this exception when compiling the query, but instead write my query differently if that feature is not available.

In most cases, I probably want to fall back to the default conflict resolution. That means, I want to leverage SKIP LOCKED if my platform supports it, but if it doesn't, I accept that I have to wait for locks to be released.

I really don't want to add another supports*() method. Falling back to the default conflict resolution method doesn't look like a fallback, it's a different behavior: the first returns some rows immediately while the second will return all but may need to wait.

If the locking behavior is acceptable, why don't you want to use it always? If not, then you should only support the databases that support skipping locked rows.

If being able to skip locked rows, if the target platform supports it, is important, it could be explicitly specified in the application configuration alongside the database connection configuration.

Yeah, makes sense. Thank you. 🙂

Co-authored-by: Herberto Graca <herberto.graca@lendable.co.uk>

stof · 2023-11-07T09:40:11Z

src/Platforms/DB2Platform.php

    /**
     * {@inheritDoc}
+     *
+     * @deprecated This API is not portable.


you should also add the Deprecation::trigger() call in this method (the call in the parent method won't be triggered as the method is overridden)

stof · 2023-11-07T09:44:53Z

src/Query/ForUpdate.php

+{
+    private int $conflictResolutionMode;
+
+    public function __construct(int $conflictResolutionMode)


This should have @psalm-param ConflictResolutionMode::* $conflictResolutionMode, and same for the property type and the return type, as well as other APIs using that pseudo-enum.

| Q | A |------------- | ----------- | Type | bug/feature/improvement | Fixed issues | Follows #6191 #### Summary This PR removed methods that have been deprecated in #6191.

morozov mentioned this pull request Oct 14, 2023

Add locking methods to the query builder #6105

Closed

morozov force-pushed the query-builder-row-lock-options branch 5 times, most recently from 924a226 to 6e891f2 Compare October 14, 2023 23:08

derrabus added this to the 3.8.0 milestone Oct 15, 2023

derrabus added New Feature QueryBuilder Deprecation labels Oct 15, 2023

morozov force-pushed the query-builder-row-lock-options branch 3 times, most recently from 266d825 to 752ebf5 Compare October 16, 2023 03:55

morozov added the Locking label Oct 16, 2023

morozov force-pushed the query-builder-row-lock-options branch 3 times, most recently from 1a518d3 to eece112 Compare October 16, 2023 04:24

morozov marked this pull request as ready for review October 16, 2023 06:23

morozov requested a review from derrabus October 16, 2023 06:23

derrabus reviewed Nov 1, 2023

View reviewed changes

morozov force-pushed the query-builder-row-lock-options branch from eece112 to ad6c848 Compare November 6, 2023 03:38

derrabus previously approved these changes Nov 6, 2023

View reviewed changes

morozov and others added 3 commits November 6, 2023 20:02

Remove unused unit test fixtures

ca9fba3

Fix coding standard violation

c32107b

Enable skipping locked rows in QueryBuilder

24735b4

Co-authored-by: Herberto Graca <herberto.graca@lendable.co.uk>

derrabus dismissed their stale review via 24735b4 November 6, 2023 19:03

derrabus force-pushed the query-builder-row-lock-options branch from ad6c848 to 24735b4 Compare November 6, 2023 19:03

derrabus merged commit c78222e into doctrine:3.8.x Nov 6, 2023
12 of 13 checks passed

morozov deleted the query-builder-row-lock-options branch November 6, 2023 19:05

xabbuh mentioned this pull request Nov 6, 2023

[Messenger] fix compatibility with Doctrine DBAL 4 symfony/symfony#52476

Merged

stof reviewed Nov 7, 2023

View reviewed changes

derrabus mentioned this pull request Nov 11, 2023

Remove deprecated lock-related methods #6219

Merged

DavidBadura mentioned this pull request Jan 29, 2024

Fix Dbal Mock issue patchlevel/event-sourcing#474

Closed

2 tasks

mollux mentioned this pull request Feb 5, 2024

Bump dependencies to align with composer based installs mautic/mautic#13311

Merged

adlpz mentioned this pull request Feb 22, 2024

Implement FOR UPDATE SKIP LOCKED doctrine/orm#7746

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable skipping locked rows in QueryBuilder #6191

Enable skipping locked rows in QueryBuilder #6191

morozov commented Oct 14, 2023 •

edited

derrabus Nov 1, 2023

derrabus Nov 1, 2023

morozov Nov 6, 2023 •

edited

derrabus Nov 6, 2023

stof Nov 7, 2023

stof Nov 7, 2023

Enable skipping locked rows in QueryBuilder #6191

Enable skipping locked rows in QueryBuilder #6191

Conversation

morozov commented Oct 14, 2023 • edited

Future scope

derrabus Nov 1, 2023

Choose a reason for hiding this comment

derrabus Nov 1, 2023

Choose a reason for hiding this comment

morozov Nov 6, 2023 • edited

Choose a reason for hiding this comment

derrabus Nov 6, 2023

Choose a reason for hiding this comment

stof Nov 7, 2023

Choose a reason for hiding this comment

stof Nov 7, 2023

Choose a reason for hiding this comment

morozov commented Oct 14, 2023 •

edited

morozov Nov 6, 2023 •

edited