-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable skipping locked rows in QueryBuilder #6191
Enable skipping locked rows in QueryBuilder #6191
Conversation
924a226
to
6e891f2
Compare
266d825
to
752ebf5
Compare
1a518d3
to
eece112
Compare
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'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test that covers the behavior of SKIP_LOCKED
for those platforms that don't support it?
|
||
if ($forUpdate->getConflictResolutionMode() === ConflictResolutionMode::SKIP_LOCKED) { | ||
if ($this->skipLockedSQL === null) { | ||
throw Exception::notSupported('SKIP LOCKED'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense. Thank you. 🙂
eece112
to
ad6c848
Compare
Co-authored-by: Herberto Graca <herberto.graca@lendable.co.uk>
ad6c848
to
24735b4
Compare
/** | ||
* {@inheritDoc} | ||
* | ||
* @deprecated This API is not portable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
{ | ||
private int $conflictResolutionMode; | ||
|
||
public function __construct(int $conflictResolutionMode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. forSELECT
,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:
getReadLockSQL()
,getWriteLockSQL()
andgetForUpdateSQL()
as non-portable.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.FOR UPDATE OF <columns>
orFOR UPDATE NO WAIT
without breaking the API.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 portableSelectQuery
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.