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

BatchUpdateException: incorrect update counts #502

Closed
reschke opened this issue Feb 2, 2016 · 28 comments
Closed

BatchUpdateException: incorrect update counts #502

reschke opened this issue Feb 2, 2016 · 28 comments

Comments

@reschke
Copy link

@reschke reschke commented Feb 2, 2016

In Apache Jackrabbit OAK, we believe we found a problem in the handling of BatchUpdateExceptions; see https://gist.github.com/reschke/226be61331653164b80a.

Summary: when executing a batch that would insert two rows, where the second row already exists, a BatchUpdateException is thrown (correct). getUpdateCounts() implies that the first operation succeeded, however, the row was not inserted.

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

What version of the driver are you using ?

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

updated code to dump DB and driver version; output is...:

DB: PostgreSQL 9.5.0
Driver: PostgreSQL Native Driver PostgreSQL 9.4.1207
update failed as expected: [1]
batch result implies that first operation (insert of key-1) suceeded
Found: key-2
Error: 'key-1' not found

@vlsi
Copy link
Member

@vlsi vlsi commented Feb 2, 2016

So the question basically boils down to "f the driver should mark all the rows in int[] executeBatch()" with Statement.EXECUTE_FAILED.

I incline to ​mark all errors with Statement.EXECUTE_FAILED in autocommit mode.
That means the application would not be able to tell which row caused the failure, however the result would be consistent with the result you observe via select.
Upd: exception message would include the index of the bad row.

In autocommit=false, int[] of [1, FAILED] should be returned.
That means, until user rollbacks, we pretend that we've modified a row.

Any thoughts? Objections?

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

On 2 February 2016 at 10:04, Vladimir Sitnikov notifications@github.com
wrote:

So the question basically boils down to "f the driver should mark all the
rows in int[] executeBatch()" with Statement.EXECUTE_FAILED.

I incline to ​mark all errors with Statement.EXECUTE_FAILED in autocommit
mode.

I agree. Makes sense if it is consistent with select

That means the application would not be able to tell which row caused the
failure, however the result would be consistent with the result you observe
via select.

In autocommit=false, int[] of [1, FAILED] should be returned.
That means, until user rollbacks, we pretend that we've modified a row.

Any thoughts? Objections?

Dave Cramer


Reply to this email directly or view it on GitHub
#502 (comment).

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

Note that the problem was discovered with autocommit=false.

I'm not sure why the situation is different for the non-autocommit case, but then the Javadoc for BatchUpdateException doesn't really say (e.g. how do partial success and SQLExpcetion relate to each other???). In our cross-DB tests, "all other" DBs did actually insert the rows for which BatchUpdateException indicated success. See http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBCTest.java?r1=1727941&r2=1727995&pathrev=1727995&diff_format=h

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

Postgres transactions are atomic. In other words if one of the statements
in the batch update fails they all will. Unless of course we create a
transaction for each update, but that kind of breaks the notion of "batch"
updates.

Curious what other databases did you test this with ?

Dave Cramer

On 2 February 2016 at 10:19, Julian Reschke notifications@github.com
wrote:

Note that the problem was discovered with autocommit=false.

I'm not sure why the situation is different for the non-autocommit case,
but then the Javadoc for BacthUpdateException doesn't really say. In our
cross-DB tests, "all other" DBs did actually insert the rows for which
BatchUpdateException indicated success. See
http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBCTest.java?r1=1727941&r2=1727995&pathrev=1727995&diff_format=h


Reply to this email directly or view it on GitHub
#502 (comment).

@kjurka
Copy link
Member

@kjurka kjurka commented Feb 2, 2016

Right, autocommit=false doesn't seem like it needs any special handling. The only option is a rollback so everything will have failed. You can't proceed or view the update that did succeed.

I think it's autocommit=true that needs special handling. The driver splits batches internally into chunks based on MAX_BUFFERED_RECV_BYTES. In autocommit=true mode, each set of statements between Sync messages becomes a transaction. So if you have a failure, some chunks (as a whole) could succeed or fail.

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

@davecramer the other DBs are: H2, Derby, Oracle, MySQL, SQLServer, and DB2. They all report a partial success and do execute the parts of the batch they marked succeeded.

I believe that if Postgres doesn't have a concept of partial success of a batch, it must not report something like that in the batch results, no matter whether autocommit is true or false.

@vlsi
Copy link
Member

@vlsi vlsi commented Feb 2, 2016

So if you have a failure, some chunks (as a whole) could succeed or fail.

That is a good point

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

This :

JDBC 4.1, section 14.1.1 says:

The commit behavior of executeBatch is always implementation-defined when
an error occurs and auto-commit is true.

speaks to when auto-commit is true. Does anyone have language around when
it is false ?

Dave Cramer

On 2 February 2016 at 10:29, Vladimir Sitnikov notifications@github.com
wrote:

So if you have a failure, some chunks (as a whole) could succeed or fail.

That is a good point


Reply to this email directly or view it on GitHub
#502 (comment).

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

a very good question indeed :-)

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

It appears to be implementation dependant.

14.1.3 says:

A JDBC driver may or may not continue processing the remaining commands in a
batch once execution of a command fails. However, a JDBC driver must always
provide the same behavior with a particular data source. For example, a
driver
cannot continue processing after a failure for one batch and not continue
processing
for another batch.
If a driver stops processing after the first failure, the array returned by
the method
BatchUpdateException.getUpdateCounts will always contain fewer entries
than there were statements in the batch. Since statements are executed in
the order
that they are added to the batch, if the array contains N elements, this
means that the
first N elements in the batch were processed successfully when executeBatch
was
called.
When a driver continues processing in the presence of failures, the number
of
elements in the array returned by the method
BatchUpdateException.getUpdateCounts always equals the number of
commands in the batch. When a BatchUpdateException object is thrown and the
driver continues processing after a failure, the array of update counts
will contain
the following BatchUpdateException constant:

I'm actually quite curious how other drivers do this in a transaction. But
that is another topic.

From above it appears that choosing not to proceed is a spec compliant
option

Dave Cramer

On 2 February 2016 at 10:34, Julian Reschke notifications@github.com
wrote:

a very good question indeed :-)


Reply to this email directly or view it on GitHub
#502 (comment).

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

Let's ignore autocommit=true for now (sorry for posting the bug with that variant).

For autocommit=false the key question is what partial success (partial failure?) means.

For the other DBs that I tried it apparently means that that part of the batch indeed was applied, and I can go on with other operations (no rollback). For PostgreSQL it currently means "I did not apply the batch, but this is the part that would have succeeded in absence of the remaining batch operations". I can understand that this is useful information, but it doesn't help at all when an application that is supposed to work with different types of databases needs to handle that exception.

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

On 2 February 2016 at 10:58, Julian Reschke notifications@github.com
wrote:

Let's ignore autocommit=true for now (sorry for posting the bug with that
variant).

For autocommit=false the key question is what partial success (partial
failure?) means.

For the other DBs that I tried it apparently means that that part of the
batch indeed was applied, and I can go on with other operations (no
rollback). For PostgreSQL it currently means "I did not apply the batch,
but this is the part that would have succeeded in absence of the remaining
batch operations". I can understand that this is useful information, but it
doesn't help at all when an application that is supposed to work with
different types of databases needs to handle that exception.

Unfortunately with postgres transaction semantics the only way this would
work the way you want it is if each statement inside the batch were
committed individually (basically auto-commit=true_. This would be much
slower and negate the whole purpose of execute batch.

Dave Cramer

@reschke
Copy link
Author

@reschke reschke commented Feb 2, 2016

@davecramer I understand that Postgres can't do the partial success thingy. In which case though it shouldn't claim it did; just report a complete failure then.

@polobo
Copy link

@polobo polobo commented Feb 2, 2016

On Tue, Feb 2, 2016 at 9:05 AM, Dave Cramer notifications@github.com
wrote:

On 2 February 2016 at 10:58, Julian Reschke notifications@github.com
wrote:

Let's ignore autocommit=true for now (sorry for posting the bug with that
variant).

For autocommit=false the key question is what partial success (partial
failure?) means.

For the other DBs that I tried it apparently means that that part of the
batch indeed was applied, and I can go on with other operations (no
rollback). For PostgreSQL it currently means "I did not apply the batch,
but this is the part that would have succeeded in absence of the
remaining
batch operations". I can understand that this is useful information, but
it
doesn't help at all when an application that is supposed to work with
different types of databases needs to handle that exception.

Unfortunately with postgres transaction semantics the only way this would
work the way you want it is if each statement inside the batch were
committed individually (basically auto-commit=true_. This would be much
slower and negate the whole purpose of execute batch.

​Don't we just need to go back and update all of the earlier "success"
items to failed as soon as one​

​fails and autocommit is false?

David J.​

@davecramer
Copy link
Member

@davecramer davecramer commented Feb 2, 2016

ah, apologies. Yes we need to mark them all failed

Dave Cramer

On 2 February 2016 at 11:12, David Johnston notifications@github.com
wrote:

On Tue, Feb 2, 2016 at 9:05 AM, Dave Cramer notifications@github.com
wrote:

On 2 February 2016 at 10:58, Julian Reschke notifications@github.com
wrote:

Let's ignore autocommit=true for now (sorry for posting the bug with
that
variant).

For autocommit=false the key question is what partial success (partial
failure?) means.

For the other DBs that I tried it apparently means that that part of
the
batch indeed was applied, and I can go on with other operations (no
rollback). For PostgreSQL it currently means "I did not apply the
batch,
but this is the part that would have succeeded in absence of the
remaining
batch operations". I can understand that this is useful information,
but
it
doesn't help at all when an application that is supposed to work with
different types of databases needs to handle that exception.

Unfortunately with postgres transaction semantics the only way this would
work the way you want it is if each statement inside the batch were
committed individually (basically auto-commit=true_. This would be much
slower and negate the whole purpose of execute batch.

​Don't we just need to go back and update all of the earlier "success"
items to failed as soon as one​

​fails and autocommit is false?

David J.​


Reply to this email directly or view it on GitHub
#502 (comment).

vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502
vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502
@vlsi
Copy link
Member

@vlsi vlsi commented Feb 2, 2016

There's one more tricky case: different SQLs in a single batch: java.sql.Statement#addBatch(String sql).

vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502
vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502
vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 2, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
@vlsi
Copy link
Member

@vlsi vlsi commented Feb 2, 2016

@reschke,

  1. Can you check if #503 solves your problem?

  2. Can you suggest a way to run Jackrabbit's tests against PostgreSQL?
    Is it mvn -P rdb-postgres .. kind of thing?
    Does it require to test all the Jackrabbit modules or there are only a few that touch PostgreSQL?

I wonder if we can add a Travis job to test snapshot version of pgjdbc against Jackrabbit's tests.

@reschke
Copy link
Author

@reschke reschke commented Feb 3, 2016

@vlsi

I'll try soonish.

Jackrabbit Oak consists of many layers, but you probably wouldn't want to test the whole stack. The interesting parts are in "oak-core", so you'd do something like:

svn co https://svn.apache.org/repos/asf/jackrabbit/oak/trunk
cd trunk/oak-core
mvn clean install -PintegrationTesting -Prdb-postgres

(with proper system property settings for JDBC URL, username, and password)
(there's also a mirror at https://github.com/apache/jackrabbit-oak)

The interesting test is in RDBDocumentStoreJDBCTest, but right now it is disabled (assumeTrue(super.dsf != DocumentStoreFixture.RDB_PG);). The production code currently works around the issue, see http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBC.java?r1=1727601&r2=1727995&pathrev=1727995&diff_format=h.

vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
vlsi added a commit that referenced this issue Feb 3, 2016
…d not get into database

Previously, executeBatch did not consider that under error condition, a part of the batch might get rolled back.

closes #502, closes #503
@vlsi vlsi closed this in #503 Feb 7, 2016
@reschke
Copy link
Author

@reschke reschke commented Mar 7, 2016

Thanks for fixing this.

In Apache Jackrabbit OAK I'd now like to take advantage of the change; but that requires removing a workaround that is present for older versions of the driver. Unfortunately, the problem is in 9.4.1207, so the change isn't detectable based on major or minor version DataBaseMetaData.

Is by any chance a 9.5 version of the driver happening soon? That would make checking the version number simpler...

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 7, 2016

Hmmm... not really. Ideally I'd like to remove the dependency on the driver
to the server. There is no real need for it.

Currently we do honour the minor version number will be increasing with
each version though.

Dave Cramer

On 7 March 2016 at 10:36, Julian Reschke notifications@github.com wrote:

Thanks for fixing this.

In Apache Jackrabbit OAK I'd now like to take advantage of the change; but
that requires removing a workaround that is present for older versions of
the driver. Unfortunately, the problem is in 9.4.1207, so the change isn't
detectable based on major or minor version DataBaseMetaData.

Is by any chance a 9.5 version of the driver happening soon? That would
make checking the version number simpler...


Reply to this email directly or view it on GitHub
#502 (comment).

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 7, 2016

@davecramer , I guess the question is "how pgdjbc-users could tell if they run .1207 vs .1208 at runtime". I think that is important, so we should probably provide API to get full version.

@reschke
Copy link
Author

@reschke reschke commented Mar 7, 2016

@vlsi a postgres-specific DatabaseMeta extension would work for me...

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 7, 2016

Uggh, I thought we already did that

Dave Cramer

On 7 March 2016 at 10:46, Julian Reschke notifications@github.com wrote:

@vlsi https://github.com/vlsi a postgres-specific DatabaseMeta
extension would work for me...


Reply to this email directly or view it on GitHub
#502 (comment).

@vlsi
Copy link
Member

@vlsi vlsi commented Mar 7, 2016

Unfortunately, DatabaseMeta does not have getPatchVersion().

@reschke, org.postgresql.jdbc.PgDatabaseMetaData#getDriverVersion would return PostgreSQL 9.4.1208. It is not super reliable, but it is already there.

However, I think it could make sense to add getPatchVersion to some PgDatabaseMetaData-specific interface.

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 7, 2016

+1

Dave Cramer

On 7 March 2016 at 10:50, Vladimir Sitnikov notifications@github.com
wrote:

Unfortunately, DatabaseMeta does not have getPatchVersion().

@reschke https://github.com/reschke,
org.postgresql.jdbc.PgDatabaseMetaData#getDriverVersion would return PostgreSQL
9.4.1208. It is not super reliable, but it is already there.

However, I think it could make sense to add getPatchVersion to some
PgDatabaseMetaData-specific interface.


Reply to this email directly or view it on GitHub
#502 (comment).

@reschke
Copy link
Author

@reschke reschke commented Mar 7, 2016

@vlsi saw that, but I'm not too enthusiastic about extracting important information using string operations...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants