Add locking methods to the query builder #6105

hgraca · 2023-07-22T17:47:39Z

Q	A
Type	feature
Fixed issues	-

Summary

Fix SQLServer getForUpdateSQL()
- It is currently returning an empty string,
  but it can be implemented with 'WITH (UPDLOCK, ROWLOCK)'.
Add lockForUpdate() to the query builder
- This will allow us to create a query that will lock the rows
  returned until the end of the transaction. This mens we can
  update these rows with the guarantee that they have not been
  changed in the meantime by a competing process.
Add skipLocked() to the query builder
- This will allow us to create a query that will not
  wait for a lock to be released on rows, and instead
  return a row set excluding the rows locked by
  another transaction.

derrabus

Thank you. Please have a look at the failing tests. As far as I can tell, some existing tests are failing due to unnecessary extra whitespaces. We should avoid those.

Furthermore, we definitely need functional tests that at least show me that this new functionality produces syntax that is understood by the database servers we support.

src/Platforms/MySQL57Platform.php

src/Platforms/MariaDb1060Platform.php

hgraca · 2023-07-23T09:32:30Z

Tkx for the CR.

Im working on fixing those whitespaces issues, and will add some functional tests, hopefully sometime this week.

hgraca · 2023-07-23T20:23:45Z

@derrabus Can you point me to a functional test like the ones you asked me to create, pls?

derrabus · 2023-07-23T20:34:32Z

Have a look at the tests/Functional/Platform directory.

derrabus · 2023-07-24T09:28:01Z

src/Id/TableGenerator.php

+                . ' WHERE sequence_name = ? '
+                . ($platform instanceof SQLServerPlatform ? '' : $platform->getWriteLockSQL());


We should avoid this kind of instanceof checks. Why are we changing this deprecated class anyway?

We changed the SQLServerPlatform::getWriteLockSQL() so that it actually spits out the right SQL, it was spitting out an empty string.
Thus, after changing it, here for MSSQL it was adding both the lock hints after the FROM (correct for MSQL) and at the end (correct for all others).
So, the test was failing and we had to change it.
Since the class is deprecated anyway, I assumes it would be fine to hack it up. Otherwise we would have to make a deep refactoring to a deprecated class, so that we would use the query builder instead of directly SQL,

Thus, after changing it, here for MSSQL it was adding both the lock hints after the FROM (correct for MSQL) and at the end (correct for all others).

But that sounds correct to me, doesn't it?

So, the test was failing and we had to change it.

Which test exactly?

We really cannot keep this, sorry. Not having platform switches all over the codebase is the reason why the platform classes exist.

Thus, after changing it, here for MSSQL it was adding both the lock hints after the FROM (correct for MSQL) and at the end (correct for all others).

But that sounds correct to me, doesn't it?

No, it should only add the extra SQL (the locking bit) either just after the from or at the end, and for MSSQL it was adding it in both locations.

So, the test was failing and we had to change it.

Which test exactly?

We really cannot keep this, sorry. Not having platform switches all over the codebase is the reason why the platform classes exist.

I don't remember what test was breaking anymore.
Now I replaced building the raw SQL (which was there already) for building the SQL with the query builder, using the locking method I added.

I left my latest changes in a fixup! commit that should be squashed before merging.

derrabus · 2023-07-24T09:30:34Z

src/Query/QueryLockParser.php

+use function rtrim;
+use function trim;
+
+final class QueryLockParser


Naming: This "parser" does not actually parse anything does it?

Well, it translates from an array of strings (the locks "names") into the appropriate SQL.
I don't like the name either, but couldn't find anything better.

Any suggestion?

I renamed it to QueryLockBuilder

hgraca · 2023-07-24T16:02:00Z

@derrabus ~~I added a functional test, but in order to test this properly, I think we need to have 2 different connections, so that we can simulate competing connections and assert the locks behaviours.~~

~~I think I didn't do it properly though, can you take a look at the last commit and tell me if there is a better way of doing this, pls?~~

hgraca · 2023-08-14T06:32:09Z

@derrabus I rebased on 3.7, renamed QueryLockParser to QueryLockBuilder, and fixed the failing functional test.

Please let me know if there is something more to be done.

hgraca · 2023-09-26T06:32:33Z

@derrabus I added a fixup to not run the test on engines that dont support 'SKIP LOCKED'.
I hope this time all the tests will be green.

src/Query/QueryLockBuilder.php

src/Types/JsonType.php

tests/Functional/Query/QueryBuilderTest.php

src/Query/QueryLockBuilder.php

tests/Driver/VersionAwarePlatformDriverTest.php

This will allow us to create a query that will lock the rows returned until the end of the transaction. This mens we can update these rows with the guarantee that they have not been changed in the meantime by a competing process.

It is currently returning an empty string, but it can be implemented with 'WITH (UPDLOCK, ROWLOCK)'.

This will allow us to create a query that will not wait for a lock to be released on rows, and instead return a row set excluding the rows locked by another transaction.

hgraca · 2023-09-28T23:33:15Z

@derrabus rebased on 3.7 and made the requested changes

hgraca · 2023-09-29T21:12:26Z

@derrabus added another fixup, hopefully now the tests are all green

src/Connection.php

morozov · 2023-10-11T14:43:04Z

@hgraca, thank you for the pull request. I slightly updated the test suite to actually run the new test against the platforms that support it and slightly modified the code. This helped me make sure that this feature is indeed worth consideration.

I would like to take it from here and give the API design a bit more thinking. If you don't mind, I will close this pull request in favor of another one and add you as a co-author and reviewer.

hgraca · 2023-10-11T17:02:45Z

Perfect, tbh I was a bit stuck with it cozz I can't run the pipeline locally, and cant figure out what's failing in the CI.

Tkx for picking this up.

morozov · 2023-10-14T22:30:29Z

Closing in favor of #6191.

hgraca force-pushed the add_locking_to_query_builder branch 2 times, most recently from 666d97c to 259c1f9 Compare July 22, 2023 20:41

derrabus added New Feature Platforms QueryBuilder Locking Failing Test Missing Tests labels Jul 22, 2023

derrabus requested changes Jul 22, 2023

View reviewed changes

src/Platforms/MySQL57Platform.php Outdated Show resolved Hide resolved

src/Platforms/MariaDb1060Platform.php Show resolved Hide resolved

hgraca force-pushed the add_locking_to_query_builder branch 3 times, most recently from b422ef1 to 295a92e Compare July 23, 2023 09:29

hgraca marked this pull request as draft July 23, 2023 16:22

hgraca force-pushed the add_locking_to_query_builder branch from 0b6bfa3 to 4a0fb32 Compare July 23, 2023 20:20

derrabus reviewed Jul 24, 2023

View reviewed changes

hgraca force-pushed the add_locking_to_query_builder branch 9 times, most recently from 4c6f913 to c008640 Compare August 13, 2023 21:04

hgraca marked this pull request as ready for review August 14, 2023 06:31

derrabus requested changes Sep 26, 2023

View reviewed changes

hgraca and others added 6 commits September 29, 2023 01:32

Add lockForUpdate() to the query builder

6677a94

This will allow us to create a query that will lock the rows returned until the end of the transaction. This mens we can update these rows with the guarantee that they have not been changed in the meantime by a competing process.

Fix SQLServer getForUpdateSQL()

53984b0

It is currently returning an empty string, but it can be implemented with 'WITH (UPDLOCK, ROWLOCK)'.

fixup!

991876d

Add skipLocked() to the query builder

e77576f

This will allow us to create a query that will not wait for a lock to be released on rows, and instead return a row set excluding the rows locked by another transaction.

fixup!

d278c6b

fixup!

02edccb

hgraca force-pushed the add_locking_to_query_builder branch from ed9eda4 to 02edccb Compare September 28, 2023 23:32

derrabus changed the base branch from 3.7.x to 3.8.x September 29, 2023 00:17

derrabus removed the Missing Tests label Sep 29, 2023

fixup!

acedc4d

DROP

3f04a41

derrabus requested changes Oct 10, 2023

View reviewed changes

src/Connection.php Show resolved Hide resolved

derrabus added this to the 3.8.0 milestone Oct 10, 2023

morozov force-pushed the add_locking_to_query_builder branch 5 times, most recently from 6d657ad to 510cc44 Compare October 11, 2023 13:55

Enable functional test

ef72694

morozov force-pushed the add_locking_to_query_builder branch from 510cc44 to ef72694 Compare October 11, 2023 14:21

morozov mentioned this pull request Oct 14, 2023

Enable skipping locked rows in QueryBuilder #6191

Merged

4 tasks

morozov closed this Oct 14, 2023

morozov removed this from the 3.8.0 milestone Oct 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add locking methods to the query builder #6105

Add locking methods to the query builder #6105

hgraca commented Jul 22, 2023

derrabus left a comment

hgraca commented Jul 23, 2023

hgraca commented Jul 23, 2023

derrabus commented Jul 23, 2023

derrabus Jul 24, 2023

hgraca Jul 24, 2023

derrabus Aug 16, 2023

hgraca Sep 14, 2023

derrabus Jul 24, 2023

hgraca Jul 24, 2023

hgraca Aug 13, 2023

hgraca commented Jul 24, 2023 •

edited

hgraca commented Aug 14, 2023

hgraca commented Sep 26, 2023

hgraca commented Sep 28, 2023

hgraca commented Sep 29, 2023

morozov commented Oct 11, 2023

hgraca commented Oct 11, 2023

morozov commented Oct 14, 2023

		. ' WHERE sequence_name = ? '
		. ($platform instanceof SQLServerPlatform ? '' : $platform->getWriteLockSQL());

Add locking methods to the query builder #6105

Add locking methods to the query builder #6105

Conversation

hgraca commented Jul 22, 2023

Summary

derrabus left a comment

Choose a reason for hiding this comment

hgraca commented Jul 23, 2023

hgraca commented Jul 23, 2023

derrabus commented Jul 23, 2023

derrabus Jul 24, 2023

Choose a reason for hiding this comment

hgraca Jul 24, 2023

Choose a reason for hiding this comment

derrabus Aug 16, 2023

Choose a reason for hiding this comment

hgraca Sep 14, 2023

Choose a reason for hiding this comment

derrabus Jul 24, 2023

Choose a reason for hiding this comment

hgraca Jul 24, 2023

Choose a reason for hiding this comment

hgraca Aug 13, 2023

Choose a reason for hiding this comment

hgraca commented Jul 24, 2023 • edited

hgraca commented Aug 14, 2023

hgraca commented Sep 26, 2023

hgraca commented Sep 28, 2023

hgraca commented Sep 29, 2023

morozov commented Oct 11, 2023

hgraca commented Oct 11, 2023

morozov commented Oct 14, 2023

hgraca commented Jul 24, 2023 •

edited