[CONJ-920] MariaDB Java Client 3.0.3 - Java batched statements with optimistic locking failing Created: 2022-01-30 Updated: 2023-12-07 Resolved: 2023-08-01 |
|
| Status: | Closed |
| Project: | MariaDB Connector/J |
| Component/s: | batch |
| Affects Version/s: | 3.0.3 |
| Fix Version/s: | 3.2.0 |
| Type: | Bug | Priority: | Major |
| Reporter: | Ronald Dehuysser | Assignee: | Diego Dupin |
| Resolution: | Fixed | Votes: | 2 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||
| Description |
|
Hi, I'm the author of JobRunr (https://github.com/jobrunr/jobrunr), a distributed job scheduler for Java. JobRunr supports quite a lot of SQL databases and after the upgrade to `org.mariadb.jdbc:mariadb-java-client:3.0.3`, none of the batched insert & update statements work anymore. These worked fine with all prior mariadb-java-clients and the same code also works fine for MySQL, Postgres, ... . How to reproduce:
|
| Comments |
| Comment by Diego Dupin [ 2022-01-31 ] | |||||||||||||||||||||||||||||
|
I've not checked in detail, but 3.0 version by default enable option `useBulkStmts` that change batch behavior. By default batch are send by bunch :
see hibernate batch verification : https://github.com/hibernate/hibernate-orm/blob/fa8b78d345e01d980a6046f226664f28799c73e0/hibernate-core/src/main/java/org/hibernate/jdbc/Expectations.java#L58 When quickly checking code about that : https://github.com/jobrunr/jobrunr/blob/master/core/src/main/java/org/jobrunr/storage/sql/common/db/Sql.java#L153 it would seems SUCCESS_NO_INFO is not checked. | |||||||||||||||||||||||||||||
| Comment by Ronald Dehuysser [ 2022-03-12 ] | |||||||||||||||||||||||||||||
|
Hi Diego, thanks for the swift response and sorry for the late reply. When I update JobRunr to also accept SUCCESS_NO_INFO as a valid SQL response (so indeed, no error occurred), less tests fail. The test https://github.com/jobrunr/jobrunr/blob/master/core/src/testFixtures/java/org/jobrunr/storage/StorageProviderTest.java#L338-L360 check whether optimistic locking works with batch jobs. However, one test still fails as the result is [-2, -2, -2 -2]. Concretely, this means I cannot know whether an update has succeeded or failed due to optimistic locking. I hope it makes sense what I say. | |||||||||||||||||||||||||||||
| Comment by Ronald Dehuysser [ 2022-03-12 ] | |||||||||||||||||||||||||||||
| Comment by Radek Wikturna [ 2023-01-06 ] | |||||||||||||||||||||||||||||
|
Hello Ironically, I reported the same bug a few years ago in driver version 2.2.6 - see https://jira.mariadb.org/browse/CONJ-630. It appeared due to the very same reason - making `useBulkStmts` the default behaviour. It was disabled by https://jira.mariadb.org/browse/CONJ-642 so my bug was closed. Now, version 3.x enables `useBulkStmts` again by default causing the same problem. I consider this a bug or at least critical incompatibility change - optimistic locking won't work in any project using Hibernate (or using the same algorithm for checking update counts). Performance enhancement cannot cause other change in functionality of another function, especially such that has impact on optimistic locking. Possible solutions: | |||||||||||||||||||||||||||||
| Comment by Radek Wikturna [ 2023-01-06 ] | |||||||||||||||||||||||||||||
|
Also, I would like to understand the option `useBulkStms` . Use dedicated COM_STMT_BULK_EXECUTE protocol for batch insert when possible. (batch without Statement.RETURN_GENERATED_KEYS and streams) to have faster batch. (significant only on >= MariaDB 10.2.7) From the documentation it appears it should affect only INSERT statements but it seems this is not true. Can you comment on that? Do you have some performance measurements measuring the following? Thanks | |||||||||||||||||||||||||||||
| Comment by Diego Dupin [ 2023-01-06 ] | |||||||||||||||||||||||||||||
|
I'll try to make this response exhaustive. Batch implementation can be complex. About COM_STMT_BULK_EXECUTE : this exchange permit batches in general, so that's mostly used for INSERT, but can be UPDATE/DELETE as well. There is multiple possible implementation for batches.
loop using TEXT protocol : driver parse command and substitute '?' by escaped parameters loop using BINARY protocol(PREPARE command already done, retuning a prepare statement id) loop using BINARY protocol, pipeliningSame than previous, but all send are done without reading server response, then reading all responses at once. contatenate TEXT commands:Send all the text command in one exchange rewrite commandsome driver (mysql) do that for INSERT command only : One response only is received, driver then knows affected rows for the total amount of affected rows. using dedicated bulk command.(PREPARE command already done, retuning a prepare statement id) To give you an idea of performance of those different implementation, executing 100 INSERT with one parameter for each insert (a string with a length of 100 chars)
( i doesn't have immediate data for "contatenate TEXT commands", but that better than looping using binary, and less than rewrite) the reason for better performance for rewrite and bulk is simple : less networks exchanges, and permit server optimisation (inserting multiple data directly for example) There is 2 restrictions then : the driver knows only success or failure, and if success, only the total number of affected rows, and latest generated Id. So that is not compatible with optimistic commands. workaround is simple, disabling option `useBulkStmts` Enabling useBulkStmts in 2.x was an error, because a major breaking change. Enabling it in 3.x make more sense, but creating this problem with optimistic commands. | |||||||||||||||||||||||||||||
| Comment by Radek Wikturna [ 2023-01-09 ] | |||||||||||||||||||||||||||||
|
I appreciate the detailed answer. I admit I don't fully understand the technical details of implementation of the batch update. First, let's create a test table // SQL Server // MariaDB then run the test.
Results: Oracle JDBC Driver 12.2.0.1.0 or 19.8.0.0.0 SQL Server JDBC Driver 7.4.1.0 MariaDB JDBC Driver 2.3.0 MariaDB JDBC Driver 3.1.0
MariaDB JDBC Driver 3.1.0, useBulkStmts=false As can be seen, both Oracle and SQL Server driver return correct and expected values by executeBatch(), Oracle also by getUpdateCount(). So, there is always some way how to find out if one or more UPDATES updated no rows due to optimistic locking condition. However, Hibernate only checks the result of executeBatch() (otherwise on each supported DB platform the implementation would have to be implemented differently) You wrote: In reality, it is far more likely to batch insert a lot of data rather then to batch update a lot of data. Therefore I think the option useBulkStmts should influence and speed up only INSERT statements, but it should not silently make optimistic locking non-functional. Alternatively, there should be 2 options useBulkStmtsForInserts (by default true), useBulkStmtsForUpdatesAndDeletes (by default false). Regards | |||||||||||||||||||||||||||||
| Comment by Diego Dupin [ 2023-01-09 ] | |||||||||||||||||||||||||||||
|
I've created a server task to add the missing feature of bulk permitting this : MDEV-30366 | |||||||||||||||||||||||||||||
| Comment by Bas de Bakker [ 2023-01-09 ] | |||||||||||||||||||||||||||||
|
While we don't use Hibernate, we have a very similar use case and have postponed upgrading the JDBC driver for this reason. I agree with Radek that it would be useful to be able to use bulk statements only for inserts and not for updates. While we haven't measured it, I expect that turning off bulk statements will have a major impact on inserts compared to the old driver with rewriteBatchedStatements=true that we use now. | |||||||||||||||||||||||||||||
| Comment by Radek Wikturna [ 2023-01-10 ] | |||||||||||||||||||||||||||||
|
I've discovered an article Batch Operations with MariaDB Connector/J which list various connection parameters (including useBulkStmts, rewriteBatchedStatements) that influence batching performance. What would be best settings for performance without loosing optimistic locking with the latest driver? This article gives performance measurements but its pretty old (old driver version was used). | |||||||||||||||||||||||||||||
| Comment by Diego Dupin [ 2023-01-10 ] | |||||||||||||||||||||||||||||
|
This can be a solution, to parse command, detect that this is an INSERT, then ensure that total of affected rows correspond to number of set of parameter, returning array of 1, not Statement.SUCCESS_NO_INFO( like rewriteBatchedStatements did). This solution is safe, even if parsing command will always have a little cost in terms of performance. Until having MDEV-30366, this seems a better default. To resume :
Still this would be a better default, even if slowing down batch of DELETE and UPDATE. |