fix: convert silent rollbacks into exception if application sends commit command #1729
Conversation
vlsi
force-pushed
the
raise_exception_on_silent_commit
branch
from
aa21fc6
to
f03b463
Compare
pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
Outdated
Show resolved
Hide resolved
vlsi
force-pushed
the
raise_exception_on_silent_commit
branch
11 times, most recently
from
5c6cc50
to
1258986
Compare
pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
Outdated
Show resolved
Hide resolved
vlsi
force-pushed
the
raise_exception_on_silent_commit
branch
5 times, most recently
from
e85aad1
to
cef17fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #1729 +/- ##
============================================
- Coverage 69.44% 66.38% -3.06%
- Complexity 4201 4238 +37
============================================
Files 187 187
Lines 17279 17803 +524
Branches 2872 3054 +182
============================================
- Hits 11999 11819 -180
- Misses 3993 4657 +664
- Partials 1287 1327 +40 |
…mit command Hopefully, server would fix the error in the future. See discussion in pgsql-hackers: https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org fixes pgjdbc#1697
vlsi
force-pushed
the
raise_exception_on_silent_commit
branch
from
cef17fb
to
8abbd4f
Compare
…mit command (#1729) The feature is enabled by default. It can be disabled by setting raiseExceptionOnSilentRollback=false connection property. See discussion in pgsql-hackers: https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org fixes #1697
|
Similar to the PR from the other day, isn't it a bit rushed to merge this in immediately? I'm not opposed to fixing this but it's a change, enabled by default, that changes transaction commit behavior. It's probably a good idea but let's at least have some review period. For example, what's the idea behind checking the SQL in the |
|
We need to release 42.2.11, and it is not clear if 42.3.0 will have -jre6 branch.
There are tests in the PR, and this SQL is present in tests. |
|
Ya I'd say we want to release. If its' broken we can fix it. Historically we've been paralyzed by lack of willingness to commit anything. Unfortunately we do not get a lot of real world testing of snapshots so the only way to test it is to release it. |
|
If you guys say so then okay. Going forward, standardizing on some standard review period would be nice. I don't know what the right answer is but it's got to be more than 24-hours for a user facing change. The thing that jumped out at me was the parsing of user's SQL commands as that has a tendency of leading to a long term baggage. I will admit that the thread about "commit" actually being a rollback might be the most surprising thing I've seen ever seen with PG. It's just plain weird. I bet most of us have never run into anything like it either as any sane code would bubble up exception. But man is that weird behavior. Though even this won't solve the original use case in the issue that spawned this thread as I think a "malicious" user that receives the Connection could do some work, fail to commit, issue a rollback, start a new transaction, then hand back the connection to the caller which things that everything is dandy. Short of actually wrapping Connection and downstream classes in proxies, I doubt it's possible to handle every edge case as they'd never know. |
|
Oh and regarding the minor style issue with sorting the properties. I've got a PR to fix that and add a sort-check test. I'll push that in a min too. |
@sehrope , can you please clarify the test case? |
|
@vlsi I mean a generic situation where the some framework code creates a connection and then hands it off to other user code. IIUC, that's what the user in the original issue was saying could happen: Say you have some generic framework code that wraps arbitrary user work in transaction, potentially with other work it wants to include in that transaction: void doWithConnection(Callable<Connection> task) {
Connection conn = ds.createConnection();
try {
conn.setAutoCommit(false);
// Do our pre-work to get things ready
doSomethingInThisTransaction(conn);
// Do the arbitrary user work that we have no control over
task.call(conn);
// User work finished successfully so do our post-work
doSomethingElseInThisTransaction(conn);
// All work finished successfully so commit...
conn.commit();
} catch (Exception e) {
// User work failed so roll back...
conn.rollback();
throw e;
}
}If the invoker of the framework invokes it with a task that performs its own commits or rollbacks, there's no guarantee to the framework that the rest of the code is executing in the same transactions: doWithConnection(conn -> {
try {
// Commit the framework transaction
conn.commit();
} catch (Exception e) {
// Ignore error and rollback
conn.rollback();
}
try {
// Do our own thing
Statement stmt = conn.createStatement();
stmt.executeQuery("INSERT ... ");
// Commit it
conn.commit();
} catch (Exception) {
// Again ignore the error
conn.rollback();
}
// Pretend we're still in the original transaction
conn.setAutoCommit(false);
});Without the framework code wrapping every JDBC interface in some kind of proxy and preventing transaction management commands, I don't see any way of totally stopping this. In most cases this is a non-issue as any sane user code would be bubble up the exceptions and cause the original transaction to fail. But it sounds like the original issue creator has run into a situation where the other code cannot be trusted and thus could muck with the transaction state. I think it may be possible for the framework to detect it's a new transaction by tracking something like the transaction XID, but once the client is handed off to the the user task, nothing short of a proxy on every interface is going to stop the arbitrary user code from being able to mess with the transaction or create its own transactions. Basically I'm saying if you can't trust the rest of your code to bubble up errors then things can can broken without you noticing anyway. That's why I don't see this break being a big deal in practice. Any sane user code would not run into it. |
|
@sehrope Just to clarify the original issue. We observed user code catching the exception (actually it was a single boundary test case of our framework), but not user code intentionally breaking the transaction into pieces using |
|
@sehrope , thanks for the cases.
|
…lbacks into exceptions on commit (pgjdbc/pgjdbc#1729) (#180) Co-authored-by: Erik Strömberg <erik.stromberg@gmail.com>
…ends commit command (pgjdbc#1729)" This reverts commit adcb194. we still want to do this but it is a breaking change and we will introduce this change in 42.3.0
See pgjdbc/pgjdbc#1729 for breaking change
Hopefully, the server would fix the error in the future.
See discussion in pgsql-hackers: https://www.postgresql.org/message-id/b9fb50dc-0f6e-15fb-6555-8ddb86f4aa71%40postgresfriends.org
fixes #1697
For documentation purposes: the feature will be enabled by default.
It can be disabled by setting
raiseExceptionOnSilentRollback=falseconnection property.Sample failure: