-
-
Notifications
You must be signed in to change notification settings - Fork 90
Implement COPY … FROM STDIN queries
#566
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
base: main
Are you sure you want to change the base?
Conversation
Sources/PostgresNIO/New/Connection State Machine/ExtendedQueryStateMachine.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift
Outdated
Show resolved
Hide resolved
fabianfett
left a comment
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.
Another review round. Thanks so much for pushing through this!
COPY … FROM STDINCOPY … FROM STDIN queries
e9aa0bd to
c5f2928
Compare
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/New/Connection State Machine/ExtendedQueryStateMachine.swift
Show resolved
Hide resolved
0e79469 to
1602d85
Compare
fabianfett
left a comment
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.
Another round. Please also make sure that merge conflicts are resolved and tests pass
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection+CopyFrom.swift
Outdated
Show resolved
Hide resolved
| /// | ||
| /// - Throws: If an error occurs during the write of if the backend sent an `ErrorResponse` during the copy | ||
| /// operation, eg. to indicate that a **previous** `write` call had an invalid format. | ||
| public func write(_ byteBuffer: ByteBuffer, isolation: isolated (any Actor)? = #isolation) async throws { |
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.
lets remove the isolation calls here and instead adopt nonisolated nonsending for the whole package.
| public func write(_ byteBuffer: ByteBuffer, isolation: isolated (any Actor)? = #isolation) async throws { | |
| public func write(_ byteBuffer: ByteBuffer) async throws { |
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.
nonisolated(nonsending) is only available in Swift 6.2, so I’m not quite sure what you’re proposing.
- Make these function only available in Swift 6.2 behind
#if compiler(>=6.2)and explicitly mark them asnonisolated(nonsending)? - Make these function
nonisolated(nonsending)when using a 6.2 compiler and let them run on the nonisolated executor context in older compilers? I’d be very worried about the subtle difference in behavior here. - Adopt the
NonisolatedNonsendingByDefaultupcoming language feature when the host compiler is 6.2+? But that just expands the problem mentioned above to all functions.
So, I’d just stick with the isolation for now.
| columns: [String] = [], | ||
| format: PostgresCopyFromFormat = .text(.init()), | ||
| logger: Logger, | ||
| isolation: isolated (any Actor)? = #isolation, |
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.
| isolation: isolated (any Actor)? = #isolation, |
Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift
Outdated
Show resolved
Hide resolved
Sources/PostgresNIO/New/Connection State Machine/ExtendedQueryStateMachine.swift
Outdated
Show resolved
Hide resolved
This implements support for COPY operations using `COPY … FROM STDIN` queries for fast data transfer from the client to the backend.
|
Thanks for the review, Fabian 🙂 Rebased, updated tests to use Swift Testing and addressed/replied to the review comments. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
==========================================
+ Coverage 75.11% 75.70% +0.58%
==========================================
Files 132 134 +2
Lines 9532 9885 +353
==========================================
+ Hits 7160 7483 +323
- Misses 2372 2402 +30
🚀 New features to boost your workflow:
|
This implements support for COPY operations using
COPY … FROM STDINqueries for fast data transfer from the client to the backend.Performance
A quick note on local performance measurements: Inserting the numbers from 0 to 1,000,000 into a table that has two columns (
INTandVARCHAR) takes ~150ms. Depending on the exact implementation, the majority of the active CPU cycles are spent converting the numbers to strings, inside string interpolation or insideByteBuffer._setBytes. If I remove the code that sends theCopyDatamessages to the backend (but keep all other logic that might incur thread hopes), the test described above takes ~50ms and utilizes the CPU at ~200%, so the real bottleneck here is the Postges backend handling the data. For comparisonpsycopg2takes 210ms with the data to be written already prepared in aStringIOobject. So, performance-wise this PR should be good to go.Ideas for follow-up PRs
PostgresCopyFromWriterto reduce the number ofCopyDatamessages we need to send (and thus the protocol overhead). Alternatively, we can leave that kind of optimization to the client.COPY FROM.Fixes #290