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

Add locking methods to the query builder #6105

Closed
wants to merge 9 commits into from

Conversation

hgraca
Copy link

@hgraca hgraca commented Jul 22, 2023

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.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/Platforms/MariaDb1060Platform.php Show resolved Hide resolved
@hgraca 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
Copy link
Author

hgraca commented Jul 23, 2023

Tkx for the CR.

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

@hgraca hgraca marked this pull request as draft July 23, 2023 16:22
@hgraca hgraca force-pushed the add_locking_to_query_builder branch from 0b6bfa3 to 4a0fb32 Compare July 23, 2023 20:20
@hgraca
Copy link
Author

hgraca commented Jul 23, 2023

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

@derrabus
Copy link
Member

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

Comment on lines 122 to 123
. ' WHERE sequence_name = ? '
. ($platform instanceof SQLServerPlatform ? '' : $platform->getWriteLockSQL());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use function rtrim;
use function trim;

final class QueryLockParser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to QueryLockBuilder

@hgraca
Copy link
Author

hgraca commented Jul 24, 2023

@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 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 hgraca marked this pull request as ready for review August 14, 2023 06:31
@hgraca
Copy link
Author

hgraca commented Aug 14, 2023

@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.

src/Query/QueryLockBuilder.php Outdated Show resolved Hide resolved
src/Types/JsonType.php Outdated Show resolved Hide resolved
tests/Functional/Query/QueryBuilderTest.php Outdated Show resolved Hide resolved
src/Query/QueryLockBuilder.php Outdated Show resolved Hide resolved
tests/Driver/VersionAwarePlatformDriverTest.php Outdated Show resolved Hide resolved
Herberto Graca and others added 6 commits September 29, 2023 01:32
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
Copy link
Author

hgraca commented Sep 28, 2023

@derrabus rebased on 3.7 and made the requested changes

@derrabus derrabus changed the base branch from 3.7.x to 3.8.x September 29, 2023 00:17
@hgraca
Copy link
Author

hgraca commented Sep 29, 2023

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

src/Connection.php Show resolved Hide resolved
@derrabus derrabus added this to the 3.8.0 milestone Oct 10, 2023
@morozov morozov force-pushed the add_locking_to_query_builder branch 5 times, most recently from 6d657ad to 510cc44 Compare October 11, 2023 13:55
@morozov morozov force-pushed the add_locking_to_query_builder branch from 510cc44 to ef72694 Compare October 11, 2023 14:21
@morozov
Copy link
Member

morozov commented Oct 11, 2023

@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
Copy link
Author

hgraca commented Oct 11, 2023

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
Copy link
Member

morozov commented Oct 14, 2023

Closing in favor of #6191.

@morozov morozov closed this Oct 14, 2023
@morozov morozov removed this from the 3.8.0 milestone Oct 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants