-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add COPY FROM
support
#500
Conversation
* feat: add copy in support for r2dbc postgresql driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your contribution. The API looks pretty decent. I added a few comments. Care to have a look?
|
||
Flux<BackendMessage> backendMessages = copyDataMessages | ||
.doOnNext(client::send) | ||
.doOnError(e -> !(e instanceof IllegalArgumentException), (e) -> sendCopyFail(e.getMessage())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for filtering IllegalArgumentException
? Generally, when a failure happens, then the upstream subscription is canceled and the copy state remains partially active without closing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, it was meant for the IllegalArgumentException thrown in startCopy but I moved the doOnError onto the backendMessages only. Thanks
.doOnNext(client::send) | ||
.doOnError(e -> !(e instanceof IllegalArgumentException), (e) -> sendCopyFail(e.getMessage())) | ||
.doOnDiscard(ReferenceCounted.class, ReferenceCountUtil::release) | ||
.thenMany(client.exchange(Mono.just(CopyDone.INSTANCE))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the client exchange, it makes sense to start with a many-sink (Sinks.Many.asFlux()
) where we keep a single conversation/publisher with the server open. Otherwise, we call exchange
multiple times and that generates a bit of overhead and may interfere with other connection activity causing protocol resynchronization.
I can apply this change during the merge as it bears some complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did an attempt but it is a little bit complex since we have to wait for the CopyInResponse before sending the data frames.
src/main/java/io/r2dbc/postgresql/api/PostgresqlConnection.java
Outdated
Show resolved
Hide resolved
src/main/java/io/r2dbc/postgresql/api/PostgresqlConnection.java
Outdated
Show resolved
Hide resolved
Are there any updates on when this'll be merged and released? |
Not yet, but I'd like to include this API for the 1.0 release. @harmvanderwal did you get a chance to build the PR locally so you can verify the API is suitable for your use case? |
I did and it works perfect for my specific use case. |
COPY FROM
support
Thank you for your contribution. That's merged and polished now. |
Add copy in support for r2dbc postgresql driver. Postgresql supports COPY statements to insert data fast: https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY
The implementation is inspired by CopyManager and QueryExecutor from https://github.com/pgjdbc/pgjdbc
Issue description
This PR contains the code for issue #183.
New Public APIs
Additional context
Note: we added a benchmark test. It turned out that reading the csv files causes much of the differences in our testing.