Batch query packets to avoid additional latency #3340
Conversation
yah certainly covered in tests already. :p That's really interesting this saves so much time. I'll run a few benches I have locally over the weekend & then ship this because that seems like an actual huge & significant speedup for things. Really sorry for the delay - was lost in some life stuff for a bit. Thanks so much for this. |
|
Side note: Node streams’ |
| packets.push(serialize.bind({ | ||
| portal: query.portal, | ||
| statement: query.name, | ||
| values: query.values, | ||
| binary: query.binary, | ||
| valueMapper: valueMapper, | ||
| })) |
There was a problem hiding this comment.
This seems to have lost the try? I’m not sure if it’s a bug in #1855’s test or if the rewrite to pg-protocol or some other change made the try unnecessary, but it should probably be consistent with prepare.
|
|
||
| if (!query.rows) { | ||
| packets.push(syncBuffer) | ||
| } |
There was a problem hiding this comment.
What about the Flush message when query.rows is present?
|
Any way we can get this this pushed into a pre-release npm version for the mean time if we are not going to get it merged into master right away please @brianc |
This fixes issue described in #3325
It seems like batching packages helps avoiding additional ~40ms latency (even on local network) for queries that require bind params.
I wrote short bench to reproduce the problem locally https://gist.github.com/thedadow451/b1ed04441bfa6fdf70a7b1c56504e988
It takes 60-70ms after this fix and ~42s without the fix.
I imagine query path is already good covered with tests and since it's just optimization and not new feature I'm not adding any more tests. (Let me know if there are any you would like to have, I can add them)