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

Db2 forupdate fix #3446

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Db2 forupdate fix #3446

merged 2 commits into from
Jul 16, 2024

Conversation

nPraml
Copy link
Contributor

@nPraml nPraml commented Jul 15, 2024

Hello Rob,

In our application we assume that the forUpdate() query locks the selected rows (that read access from another thread is no longer possible). It works on MariaDb and SqlServer, but not on Db2.

The DB2 documentation states that the rows are locked but are not protected against write access: https://www.ibm.com/docs/en/db2/11.5?topic=statement-update-clause

We found out that if you add an orderBy("id") to the query, the locking also works under DB2. In our opinion it is just a workaround and not the solution. (And we do not understand why)

We found another way for DB2 forUpdate() to lock the rows: with rs use and keep update locks

Can you please look at the PullRequest and give us feedback?

Best regards
Noemi

@rPraml
Copy link
Contributor

rPraml commented Jul 15, 2024

Did we use "forUpdate" in a wrong way?
We used it for synchronizing two or more application-instances when accessing a certain piece of code. In the special use case we have a shared NFS directory where only one instance should access this directory.

This was done with a

try(Transaction txn = DB.beginTransaction()) {
   List configs = DB.find(DirectoryScanConfig).where(...).forUpdate().findList(); // other thread will block here
   // scan the directory in the config(s)
}

Now we found out, that this might be the wrong way. Reasons:

  • it does not work in DB2 as we expected. And we cannot explain, why it would work with order by id - does DB2 lock the index?
  • BTW: it also does not work on H2 as there is a default lock timeout of 2s. So an other thread will block only for this time (see test)

Nevertheless, we need something to do a synchronization over DB.
@rbygrave - if we misunderstand the purpose of forUpdate, what do you think, if we extend the API with a query.lockRows() or optionally query.lockRows(timeout) that can be used to implement this kind of locking. It can be implemented with for update by default, maybe with an infinity timeout on H2 or with rs use and keep lock on DB2

FYI: This locking is also used on DbMigration for some platforms


start = System.currentTimeMillis() - start;

assertThat(start).isGreaterThan(2800);
Copy link
Contributor

Choose a reason for hiding this comment

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

here we check, if the second thread blocks, while the first thread locks the data for at least 3000 ms.

@@ -57,6 +57,6 @@ public PlatformIdGenerator createSequenceIdGenerator(BackgroundExecutor be, Data
@Override
protected String withForUpdate(String sql, Query.LockWait lockWait, Query.LockType lockType) {
// NOWAIT and SKIP LOCKED not supported with Db2
return sql + " for update";
return sql + " with rs use and keep update locks";
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbygrave
Copy link
Member

rbygrave commented Jul 16, 2024

Did we use "forUpdate" in a wrong way?

No, you used it in the way I would expect.

it does not work in DB2 as we expected

Yes, DB2 is taking its own approach/syntax here imo (which I think is confusing).

This locking is also used on DbMigration for some platforms

Not for DB2 though, DB2 is using "LogicalLocking" for DbMigration.

So yes, I'm happy with this PR.

@rbygrave rbygrave added the bug label Jul 16, 2024
@rbygrave rbygrave added this to the 14.5.0 milestone Jul 16, 2024
@rbygrave rbygrave merged commit 9f0eaef into ebean-orm:master Jul 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants