Uh oh!
There was an error while loading. Please reload this page.
features/sql: read result sets with plain JDBC instead of rxjava3-jdbc - #625
Open
cportele wants to merge 1 commit into
Open
features/sql: read result sets with plain JDBC instead of rxjava3-jdbc#625cportele wants to merge 1 commit into
cportele wants to merge 1 commit into
Conversation
Streamed reads (a positive fetch size, used by single-shot queries) ran in a rxjava3-jdbc transacted select, which commits and releases the connection only once the rows are exhausted. An error or a cancellation - the read stall timeout, a client that went away - left the connection leased for good, and enough of them starve the pool. All reads now lease a connection from the pool and stream the result set with Flowable.using and Flowable.generate: statement, result set and connection are released on completion, error and cancellation alike. A read with a fetch size still runs in a transaction so the driver uses a server-side cursor; it is ended with a rollback. Errors raised while rows are read reach the consumer. rxjava3-jdbc is no longer used: the dependency, the Database in the connector and the unused SqlDbmsAdapter.getRxType() are removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the last variant of the connection leak behind ldproxy/ldproxy#1761 — on the read path this
time — and, with no user left, removes the rxjava3-jdbc dependency. This goes beyond
ldproxy/ldproxy#1645, whose "remove usage of rxjava3-jdbc" concerns the SQL mutations and is
covered by #624.
Problem
Streamed reads — a positive fetch size, as used by single-shot queries — ran in a rxjava3-jdbc
transacted select, which commits and releases the connection only once the rows are exhausted.
An error while reading, or a cancellation (the read-stall timeout from #606, a client that went
away), left the connection leased for good.
Change
All reads lease a connection from the pool and stream the result set with
Flowable.using+Flowable.generate; statement, result set and connection are released on completion, error andcancellation alike. A read with a fetch size still runs in a transaction so the driver uses a
server-side cursor (ended with a rollback, the cheapest way to close the cursor). Everything
reactive is unchanged: backpressure, execution on the subscribing thread,
subscribeOnforparallel single-shot sub-queries, the read-stall timeout, the SQL result logging.
Differences to the library's
Select: errors raised while rows are streamed now reach the consumer(the library swallowed them, which is what the transacted read was working around), resources are
released before completion is signalled downstream, and query strings are no longer scanned for
?/:nameplaceholders (ldproxy never binds parameters).With no user left, the rxjava3-jdbc dependency, the
DatabaseinSqlConnectorRxand the unusedSqlDbmsAdapter.getRxType()are removed.Why the library is not missed
rxjava3-jdbc is a general-purpose reactive JDBC wrapper; ldproxy used a narrow slice of it, and
that slice is exactly what the ~60 lines in
SqlClientRxnow do directly. Feature by feature:Pools.nonBlocking())Databasewas created withfromBlocking(hikariDataSource), so every read ran synchronously on the subscribing threadsubscribeOnwhere parallelism is wanted?,:name, collection expansion, statement reuse across parameter groups)?, e.g. the JSONB?operator, is gone)ResultSetMapper)SqlRowVals.read(ResultSet, options)transacted(),Tx,dependsOn,returnGeneratedKeys)setAutoCommit(false)+setFetchSizedo that in two linesCall), pool health-check queries (DatabaseType)getRxType()had no caller)Flowable.using(prepare, generate overrs.next(), close)— the same structure as the new code — but with the transacted variant's reference counting on top, which releases only on natural completionFlowable.using+Flowable.generatewithout the counter: release on completion, error and cancelSo the library provided no capability the read path used that plain JDBC under RxJava does not
provide equally; what it added on top was the reference-counted transaction handling that caused
the leak, an SQL placeholder parser we did not need, the swallowed streaming error (#1293), and a
fork (
0.1.4-ii.1) to maintain. The reactive contract of the read path — a backpressuredFlowable<SqlRow>, one connection per subscription, cancellation from downstream — is provided byRxJava itself and is unchanged.
Verification
SqlClientRxSpec: streamed and paged reads release statement, result set and connection oncompletion, on cancellation, on a failing
next(), on a failing execute and on a failed lease;run()and session paths.unchanged; six large reads aborted by a throttled client mid-stream returned their connection
every time (pool at baseline, HikariCP total equal to the database's backend count); the
search/result-set and stored-query smoke tests — which include a header-only read of a
100k-feature stored query, i.e. a cancelled streamed read — and the transaction smoke tests pass.