"Convert silent rollbacks into exception if application sends commit command" breaks client code #1739
Comments
|
Well we aren't likely to revert this change. You should either fix your code to expect an exception or disable the feature using raiseExceptionOnSilentRollback=false. That said it is possible that the server will be changed to throw an error as this discussion is ongoing. In which case we may remove the option to disable it. |
|
It's fine by me, I just wanted to give you heads up. The same code works in all other databases and older Postgres JDBC drivers so you can expect lots of error reports like this. |
|
Why are you committing at that point anyway? It's possible that the transaction threw an exception. |
Technically speaking, void commit() throws SQLException;That is why you must be prepared to handle
Exactly. PostgreSQL follows a model where any single exception marks the transaction as an aborted one, so the database forbids patterns like catch (DuplicateKeyException) {
// lock record exists, proceed to update
}Note: your comment suggests "proceed to update", however, in practice you can never proceed to update. Even older driver was not able to update a record after catching SQLException. The thing is the database silently rolls back the transaction, so the old behavior was to rollback the transaction. So the transaction looked like a successful one, however, it was rolled back. Now we make the failure explicit, so, yes, it is expected that everybody who adopts PostgreSQL must be prepared to write exception-less SQL code, or they must ask PostgreSQL developers to allow Oracle-like behavior. Note: |
|
TL;DR: the exception is expected. If you see it you need to double-check why the code is trying to commit a failing transaction. If you want to continue the transaction even in case of SQLExceptions, then you need to configure |
|
The thing is that developers working on higher level code usually do not commit the transaction manually but by using a library. For example
You are basically breaking all such libraries, but if it's according to the spec then I guess it does not matter. |
|
Well given that Oracle (and others) do throw an exception when commit fails, how do these frameworks deal with this. Also looking at the spring template it is prepared to throw a transaction exception. No idea what Kotlin is doing there. |
The question here is what do application developers expect when they catch SQLException within the application code. For instance, you have this code: https://github.com/lukas-krecan/ShedLock/blob/57211b0a6f74b0b9a38206b14b109670df12da99/shedlock-core/src/main/java/net/javacrumbs/shedlock/support/StorageBasedLockProvider.java#L86-L87 if (storageAccessor.insertRecord(lockConfiguration)) {
lockRecordRegistry.addLockRecord(name);
// we were able to create the record, we have the lock
return true;
}
// we were not able to create the record, it already exists, let's put it to the cache so we do not try again
lockRecordRegistry.addLockRecord(name);In practice, I know it might be extremely unexpected (e.g. when coming from Oracle DB background), but it is the way PostgreSQL works. So if you want to work with PostgreSQL, you have two options: b) Use c) Write a post to pgsql-hackers mailing list, and ask PostgreSQL developers to provide a compatibility mode (e.g. avoid failing the transaction on each and every exception) |
|
Just in case: I believe what we discuss here is a true Shedlock bug that was previously hidden. PS. @lukas-krecan , if you like, we can try adding a Travis CI job that would run Shedlock against each pgjdbc head commit as an integration test. |
|
I was just trying to help, I am able to change my code, I just wanted to warn you that you are potentially breaking lot of stuff in minor release, that's all. Thanks for your time. |
|
@lukas-krecan , thanks for raising the question, however this change in behavior was exactly what we planned. It would still be great to know if there are false positives, however, so far it looks like your case is true positive. |
|
What about code like this: @org.springframework.transaction.annotation.Transactional
public void insert() {
try {
jdbcTemplate.update("insert into my_table values('test')");
} catch(DataAccessException e) {
// ignored, expected behavior
logger.info("action=error_on_insert {}", e.getMessage());
}
}The flow is like this:
In order to use 42.2.11, all users would have to move exception handling out of their transactions or more likely, Spring would somehow have to detect that it's supposed to rollback the transaction. The demo code is available here https://github.com/lukas-krecan/datasource-demo |
Exactly. What do you expect? For instance: what if you add a REST controller that calls this Do you really expect that PS. Thanks for taking the time for creating a reproducer. |
|
PS. An application can mark a transaction as a rollback-only via @Resource
private SessionContext context;
context.getRollbackOnly(); |
|
I do not dispute correctness or incorrectness. What I am trying to say is that such change deserves to be released in major version to warn people that their code might break. |
|
This is debatable as it is quite likely that the server is going to make the same change and all versions of the driver will have this behaviour. |
Ok. that is a different story. We can indeed re-brand 42.2.11 as 42.3.0. However, the change is mentioned twice in the |
I'm submitting a ...
Describe the issue
Changed introduced by #1729 breaks ShedLock and most likely other clients as well
ShedLock tries to coordinate scheduled tasks using a database lock. In pseudocode it works like this:
Driver Version?
42.2.11
Java Version?
PostgreSQL Version?
10.6
To Reproduce
Steps to reproduce the behaviour:
Checkout https://github.com/lukas-krecan/ShedLock/pull/new/postgre-bug
Run net.javacrumbs.shedlock.provider.jdbctemplate.PostgresJdbcTemplateLockProviderIntegrationTest
Expected behaviour
To not throw exception on commit.
Logs
Related issue: lukas-krecan/ShedLock#207
The text was updated successfully, but these errors were encountered: