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.

Sign up for GitHub

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

Jump to bottom

Ensure queries respect fetch size #299

Merged
merged 5 commits into from
Feb 17, 2022
Merged

Ensure queries respect fetch size #299

merged 5 commits into from
Feb 17, 2022

Conversation

jmf-tls
Copy link
Member

@jmf-tls jmf-tls commented Feb 15, 2022 •
edited
Loading

When querying the database with a fetch size set, some DB engines ignore that setting and get all results into memory, which might crash the client code due to out of memory, or at least cause long GC pauses.

This commit makes PostgreSQL and CockroachDB respect the fetch size, by wrapping the queries in a transaction if needed, so that a server side cursor is kept while the results are being fetched.

In the case of MySQL, server side cursors can be enabled so that it respects the fetch size, but due to bug https://bugs.mysql.com/bug.php?id=106465 this would make the driver ignore the query timeouts.

upsidedownsmile
upsidedownsmile requested changes Feb 16, 2022
View reviewed changes
src/main/java/com/feedzai/commons/sql/abstraction/dml/result/ResultIterator.java Show resolved Hide resolved
src/main/java/com/feedzai/commons/sql/abstraction/dml/result/ResultIterator.java Outdated Show resolved Hide resolved
src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/TestRouter.java Show resolved Hide resolved
src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/TestRouter.java Show resolved Hide resolved
src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/TestRouter.java Show resolved Hide resolved
src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/TestRouter.java Show resolved Hide resolved
src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/TestRouter.java
*
* @author José Fidalgo (jose.fidalgo@feedzai.com)
*/
public class TestRouter implements Closeable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't TestProxy be a more appropriate name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe 😄

src/test/java/com/feedzai/commons/sql/abstraction/engine/impl/abs/AbstractEngineSchemaTest.java Outdated Show resolved Hide resolved
@upsidedownsmile
Copy link
Collaborator

According to the MySql support team, 8.0 doesn't have this bug. So are we planning to not support 5.1 anymore?

@jmf-tls
Copy link
Member Author

jmf-tls commented Feb 16, 2022

According to the MySql support team, 8.0 doesn't have this bug. So are we planning to not support 5.1 anymore?

I checked, and it doesn't have the bug.
We are not currently using MySQL, but I we should keep support in PDB, I guess...
We could split this into a PDB API module and then implementation modules, that way we could have a mysql5 and a mysql8, each with a different driver version and different capabilities.
Or we could bump the driver version, adjust the code, and release a major version.

@upsidedownsmile
Copy link
Collaborator

According to the MySql support team, 8.0 doesn't have this bug. So are we planning to not support 5.1 anymore?

I checked, and it doesn't have the bug. We are not currently using MySQL, but I we should keep support in PDB, I guess... We could split this into a PDB API module and then implementation modules, that way we could have a mysql5 and a mysql8, each with a different driver version and different capabilities. Or we could bump the driver version, adjust the code, and release a major version.

MySql 5.7 EOL will be next year(October 21, 2023) so maybe we could just support it until then and after just support 8.0

@jmf-tls jmf-tls mentioned this pull request Feb 17, 2022
Open
@jmf-tls jmf-tls merged commit fa3d23e into master Feb 17, 2022
@jmf-tls jmf-tls deleted the ft-ze-ensure_fetchsize branch February 17, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugomiguelabreu hugomiguelabreu hugomiguelabreu left review comments

@artpdr artpdr artpdr approved these changes

@upsidedownsmile upsidedownsmile upsidedownsmile approved these changes

@ctosin ctosin ctosin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jmf-tls @upsidedownsmile @hugomiguelabreu @artpdr @ctosin