Add MySql LOCAL INFILE functionality #2738
Conversation
|
I like the functionality of LocalInfileHandler that doesn't tie the local implementation to a local file, just a handler to push data. This should mitigate the risks of enabling localinfile as default in the protocol. |
jelle-bigbridge
force-pushed
the
add-local-infile
branch
from
0cc0db7 to
b3b4d10
Compare
jelle-bigbridge
force-pushed
the
add-local-infile
branch
from
380288b to
d09f675
Compare
jelle-bigbridge
force-pushed
the
add-local-infile
branch
4 times, most recently
from
03a7c85 to
7f016e8
Compare
jelle-bigbridge
force-pushed
the
add-local-infile
branch
from
7f016e8 to
f9cf34e
Compare
|
@abonander could you look at the changes I made? |
| let mut send = SendPacket::new(buf, self.stream.sequence_id); | ||
| self.stream.sequence_id = self.stream.sequence_id.wrapping_add(1); | ||
| // Try to poll the send future right now | ||
| match Pin::new(&mut send).poll_send(cx, self.stream.socket_mut()) { |
There was a problem hiding this comment.
Why are you bypassing the connection's internal buffer?
There was a problem hiding this comment.
My idea was that you either use the "easy" way of calling MySqlLocalInfile::send a couple times, which does buffering, or you want to do a different style of buffering so you need a "direct" API. In my own usage, I wrap the "direct" writer with my own BufWriter to provide buffering of infile packets.
There was a problem hiding this comment.
My thought is, we already have a perfectly buffer in the connection. Why force the user to allocate another?
There was a problem hiding this comment.
@jelle-bigbridge Do you plan to get back to working on this PR? This feature would be very useful for us, if you're busy I could pick it up.
There was a problem hiding this comment.
I need feedback from the maintainers to finish this to be quite honest. If this direct mode needs to be reworked I can do it, but I need some direction to take this in. I don't want to go and make changes myself without knowing if it will help to get it merged
There was a problem hiding this comment.
@pingiun my feedback was to not bypass the connection's internal buffer.
There was a problem hiding this comment.
For copying from an AsyncRead type, you can include a read_from method similar to PgCopyIn: https://docs.rs/sqlx/latest/sqlx/postgres/struct.PgCopyIn.html#method.read_from
The trait situation isn't great there, fixing it depends on having async I/O traits in std, traits which I was told were coming more than two years ago and still haven't materialized: #1669 (comment)
|
Sorry for answering with my personal account @pingiun. I plan to migrate my work account to personal in the future. |
Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
Co-authored-by: Austin Bonander <austin.bonander@gmail.com>
|
@abonander what is needed to get this merged? |
| ready!(socket.poll_write_ready(cx))?; | ||
| } | ||
| Ok(written) => { | ||
| this.buf.drain(..written); |
There was a problem hiding this comment.
This is going to copy contents of the buffer on every poll
There was a problem hiding this comment.
From the docs it doesn't seem like drain should copy the contents of the buffer, can you explain?
There was a problem hiding this comment.
drain removes specified range of elements. But Vec<_> cannot have holes, so when you remove elements, it will move remaining elements in order to fill the hole.
In this case, buf could be 1 MB for example. May be socket only managed to write 10 KB (written = 10_000). When we call buf.drain(..10_000) it'll remove the first 10_000 bytes from the vector by moving the remaining 990_000 bytes to the beginning of the vector.
There was a problem hiding this comment.
This is another reason not to reinvent the wheel with buffering here. The connection already has a buffer that doesn't have this problem.
Fixes #1766
I've added a test which confirms this functionality. I was unsure about the function name
local_infile_statementand the trait nameMySqlExecutorInfileExt. Please tell me if you want to have different names