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

Allow to set arbitrary Integer values to FetchSize #79

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

michele-mancioppi
Copy link
Contributor

Rework the fetchSize parameter to allow the specification of negative integers
like Integer.MIN_VALUE, which is needed in some typoes of queries.

See https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-reference-implementation-notes.html

Closes #78

Rework the fetchSize parameter to allow the specification of negative integers
like Integer.MIN_VALUE, which is needed in some typoes of queries.

See https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-reference-implementation-notes.html

Closes davidmoten#78
@davidmoten
Copy link
Owner

davidmoten commented Jun 27, 2017

Thanks for the PR.

Why change fetchSize from int to Integer? Does fetchSize of 0 have meaning?

I also haven't understood what is the difference between setting fetchSize to Integer.MIN_VALUE and setting fetchSize to 1. What do you think is the difference?

@davidmoten
Copy link
Owner

davidmoten commented Jun 27, 2017

I just read

https://stackoverflow.com/questions/26046234/is-there-a-mysql-jdbc-that-will-respect-fetchsize/36986532#36986532

If you enable the MySQL JDBC option useCursorFetch, fetchSize will indeed be respected by the driver.

However, there is one disadvantage to this approach: It will use server-side cursors, which in MySQL are implemented using temporary tables. This will mean that results will not arrive until the query has been completed on the server, and that additional memory will be used server-side.

If you just want to use result streaming and don't care about the exact fetch size, the overhead of setFetchSize(Integer.MIN_VALUE) is not as bad as the docs might imply. It actually just disables client-side caching of the entire response and gives you responses as they arrive; there is no round-trip per row required.

On this basis I would continue using 0 for not set, allow Integer.MIN_VALUE as a valid fetchSize and not allow any other negative value. What do you think?

@davidmoten
Copy link
Owner

for mysql it looks like fetchSize=Integer.MIN_VALUE enables backpressure at the TCP socket:

With MIN_VALUE, MySQL seems to rely on the underlying TCP socket's flow control. As long as the receive window is reasonably sized (and all modern OSes do that by default), you should never have to wait for more data except if your read is bandwidth-limited in the first place

@litalk
Copy link

litalk commented Jun 27, 2017

Great! if it could be merged soon it would be very appreciated.

@michele-mancioppi
Copy link
Contributor Author

It seems that drivers take some latitude with setFetchSize and the values set in there. MySQL is an example, but I dug a bit in proprietary databases on my side and apparently we do some "creative" interpretations there too :-) So I am no longer very optimistic about using 0 as default.

If we were on Java 8, I'd have used an Optional, but to keep the PR simple I just went with the boxed version of int to have the "null as not set" semantic.

@michele-mancioppi
Copy link
Contributor Author

In other words, if we go with "0 is default, Integer.MIN_VALUE is the only negative int accepted" we risk having another issue about this the next time a quirky driver comes along.

@davidmoten
Copy link
Owner

In other words, if we go with "0 is default, Integer.MIN_VALUE is the only negative int accepted" we risk having another issue about this the next time a quirky driver comes along.

I'm happy with your argument. I'll merge and release.

@davidmoten davidmoten merged commit bc3d464 into davidmoten:master Jun 27, 2017
@davidmoten
Copy link
Owner

released to Maven Central 0.7.7

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.

None yet

3 participants