Skip to content

Conversation

@findepi
Copy link
Contributor

@findepi findepi commented May 10, 2021

Transactions are not supported, so setAutoCommit(false) should fail,
as it does not provide the isolation or rollback-ability requested by
the caller.

commit() and rollback() should fail in auto-commit mode (this is
what PostgreSQL JDBC driver does).

findepi added 2 commits May 10, 2021 16:26
Transactions are not supported, so `setAutoCommit(false)` should fail,
as it does not provide the isolation or rollback-ability requested by
the caller.

`commit()` and `rollback()` should fail in auto-commit mode (this is
what PostgreSQL JDBC driver does).
@github-actions
Copy link

Benchmark                           (client)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc       normal  thrpt   20   287.183 ±  28.857  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20   283.573 ±  28.149  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc       normal  thrpt   20  1770.757 ± 220.072  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20  1857.534 ± 240.282  ops/s

@zhicwu zhicwu changed the base branch from master to develop May 10, 2021 21:45
@Override
public void rollback() throws SQLException {

throw new SQLException("Cannot commit when auto-commit is enabled");
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. Should be rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, changed

@zhicwu
Copy link
Contributor

zhicwu commented May 10, 2021

Thanks @findepi. We probably need an option to make it acting like a full featured driver, so that it will work with more clients.

1 similar comment
@zhicwu
Copy link
Contributor

zhicwu commented May 10, 2021

Thanks @findepi. We probably need an option to make it acting like a full featured driver, so that it will work with more clients.

@findepi
Copy link
Contributor Author

findepi commented May 11, 2021

Implementing transactions is obviously a good thing to do, but it's not something one can do within the driver itself, without support on the server side:

For example, consider doing a "simulation":

conn.setAutoCommit(false); // start transaction
conn.createStatement().executeUpdate("INSERT .... "); 
// SELECT issued now should see effects of INSERT
conn.rollback();
// SELECT issued now should NOT see effects of INSERT

With current implementation (as visible on master), setAutoCommit and rollback are ignored, so the code doesn't throw, but is not correct, as the INSERT's effects are visible after rollback.

I understand, however, that some applications may be reliant on this incorrect behavior (for example some JPA systems may be tempted to use transactions unconditionally). @zhicwu Is this what you're concerned about?

@github-actions
Copy link

Benchmark                           (client)  (statement)   Mode  Cnt     Score     Error  Units
Basic.insertOneRandomNumber  clickhouse-jdbc       normal  thrpt   20   264.174 ±  23.552  ops/s
Basic.insertOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20   258.423 ±  26.248  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc       normal  thrpt   20  1289.935 ± 195.114  ops/s
Basic.selectOneRandomNumber  clickhouse-jdbc     prepared  thrpt   20  1361.872 ± 191.656  ops/s

@zhicwu
Copy link
Contributor

zhicwu commented May 11, 2021

Is this what you're concerned about?

Yes. And you're right that the behavior is incorrect. Having said that, it might be helpful by providing the option so that people can choose. It's still an immature thought but we may get back to this later.

Anyway, thanks again for the pull request, I have no problem to merge it into next release, cheers!

@zhicwu zhicwu merged commit 5a5fb7d into ClickHouse:develop May 11, 2021
@findepi findepi deleted the findepi/return-proper-error-when-user-tries-to-enable-transactions-5e522f branch May 11, 2021 13:27
@findepi
Copy link
Contributor Author

findepi commented May 11, 2021

thanks for a quick merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants