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

feat: support for large update counts (JDBC 4.2) #935

Merged
merged 1 commit into from Oct 1, 2019

Conversation

jorsol
Copy link
Member

@jorsol jorsol commented Sep 5, 2017

PostgreSQL do support sending large counts for statements, and Java 8 have new methods that are not implemented in the driver.

This is an implementation for large counts and implement some methods like:

  • PreparedStatement.executeLargeUpdate()
  • Statement.executeLargeUpdate(String sql)
  • Statement.getLargeUpdateCount()
  • Statement.executeLargeBatch()
  • Statement.executeLargeUpdate(String sql, int autoGeneratedKeys)
  • Statement.executeLargeUpdate(String sql, int[] columnIndexes)
  • Statement.executeLargeUpdate(String sql, String[] columnNames)

Most methods are copies of the non-Large versions, so they should behave equals with just the difference of returning long instead int.

@codecov-io
Copy link

codecov-io commented Sep 5, 2017

Codecov Report

Merging #935 into master will increase coverage by <.01%.
The diff coverage is 77.94%.

@@             Coverage Diff              @@
##             master     #935      +/-   ##
============================================
+ Coverage     68.67%   68.68%   +<.01%     
- Complexity     3891     3902      +11     
============================================
  Files           179      179              
  Lines         16391    16415      +24     
  Branches       2669     2673       +4     
============================================
+ Hits          11256    11274      +18     
- Misses         3887     3891       +4     
- Partials       1248     1250       +2

@jorsol jorsol changed the title WIP: feat: initial support large update count methods (JDBC 4.2) feat: initial support large update count methods (JDBC 4.2) Sep 20, 2017
@jorsol jorsol changed the title feat: initial support large update count methods (JDBC 4.2) feat: support for large update counts (JDBC 4.2) Sep 22, 2017
@vlsi vlsi added the triage/needs-review Issue that needs a review - remove label if all is clear label Sep 25, 2017
@jorsol jorsol force-pushed the initial-largeupdate branch 2 times, most recently from ebbdbcf to d34b4a7 Compare October 4, 2017 20:38
@jorsol jorsol force-pushed the initial-largeupdate branch 2 times, most recently from 601d48f to 2e6cdc4 Compare October 9, 2017 14:13
@jorsol
Copy link
Member Author

jorsol commented Oct 9, 2017

This feature is ready for 42.2.0, comments are welcome.

@davecramer
Copy link
Member

Help me out to understand why you think this would be 42.2.0 ? Is this a new feature ? To me this seems to just be fixing a potential bug ?

@vlsi
Copy link
Member

vlsi commented Oct 10, 2017

  1. This is a new API (=> feature)
  2. The results for existing APIs should be intact (=> not a bug)
  3. This might break existing APIs (=>safer to put in a 2.0)

I agree with @jorsol version analysis

@jorsol
Copy link
Member Author

jorsol commented Oct 10, 2017

Yes, is a new feature, in fact is a feature that must be implemented for JDBC 4.2.

  1. This might break existing APIs (=>safer to put in a 2.0)

In this case, it's hard that this break any existing APIs and for new features, it make sense to put in a minor release in a backwards-compatible manner, since this is not a patch or bug-fix.

Copy link
Member

@vlsi vlsi left a comment

Choose a reason for hiding this comment

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

Code and tests looks good to me except a couple of comments.

return uncompressUpdateCount();
public long[] getLargeUpdateCount() {
long[] copyCount = uncompressLongUpdateCount();
return Arrays.copyOf(copyCount, copyCount.length);
Copy link
Member

Choose a reason for hiding this comment

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

Is copy required here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really required, I guess there are no side effects of exposing the original array, so I will change that part.


//#if mvn.project.property.postgresql.jdbc.spec >= "JDBC4.2"
@Override
public long executeLargeUpdate() throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

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

Would you please co-locate this method with int executeUpdate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

Assert.fail("A result was returned when none was expected. Returned: " + count);
} catch (SQLException e) {
// TOO_MANY_RESULTS("0100E"), is this the correct SQLState?
Assert.assertEquals("0100E", e.getSQLState());
Copy link
Member

Choose a reason for hiding this comment

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

It's better use org.postgresql.util.PSQLState enum values, however I TOO_MANY_RESULTS sounds strange for that case indeed

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use the PSQLState.

My question was because I have not found anywere this status code, it's not part of PostgreSQL error codes, and belongs to the Class 01- Warning group, so I guess is made up just for the driver use but is just strange, this is how the driver handle it since long time so I don't touch that part.

@vlsi vlsi added waiting-for-changes and removed triage/needs-review Issue that needs a review - remove label if all is clear labels Dec 16, 2017
@jorsol jorsol force-pushed the initial-largeupdate branch 2 times, most recently from 5291eaa to f416fea Compare December 18, 2017 16:45
@jorsol
Copy link
Member Author

jorsol commented Dec 21, 2017

Rebased and ready for review.

@jorsol
Copy link
Member Author

jorsol commented Jan 23, 2018

Should this be included in 42.2.1?

@vlsi
Copy link
Member

vlsi commented Jan 24, 2018

This sounds like 42.3.0

@jorsol
Copy link
Member Author

jorsol commented Jan 24, 2018

Ok, fair enough.

@vlsi
Copy link
Member

vlsi commented Jul 12, 2018

@jorsol , would you please rebase on top of master?

@vlsi vlsi added this to the 42.3.0 milestone Jul 12, 2018
@jorsol
Copy link
Member Author

jorsol commented Jul 12, 2018

@jorsol , would you please rebase on top of master?

Sure, give me a day or two 🙂

@jorsol
Copy link
Member Author

jorsol commented Jul 12, 2018

Rebased.

@vlsi vlsi added compat-jdbc-spec enhancement triage/needs-review Issue that needs a review - remove label if all is clear and removed waiting-for-changes labels Jul 12, 2018
@davecramer
Copy link
Member

Seems reasonable. We should aim at pushing this after resolving the conflicts.

Added support for large update counts of the JDBC 4.2 API (Java 8+)

This implementation supports:
PreparedStatement.executeLargeUpdate()
Statement.executeLargeUpdate(String sql)
Statement.getLargeUpdateCount()
Statement.executeLargeBatch()
Statement.executeLargeUpdate(String sql, int autoGeneratedKeys) *
Statement.executeLargeUpdate(String sql, int[] columnIndexes) *
Statement.executeLargeUpdate(String sql, String[] columnNames) *
@AppVeyorBot
Copy link

Build pgjdbc 1.0.465 completed (commit 9161b21b1a by @jorsol)

@davecramer
Copy link
Member

@jorsol is this ready to go ?

@jorsol
Copy link
Member Author

jorsol commented Oct 1, 2019

Yes, this should be ready.

@davecramer davecramer merged commit 0888e93 into pgjdbc:master Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat-jdbc-spec enhancement triage/needs-review Issue that needs a review - remove label if all is clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants