Skip to content
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

Batch query packets to avoid additional latency #3340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thedadow451
Copy link

@thedadow451 thedadow451 commented Nov 1, 2024

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)

@brianc
Copy link
Owner

brianc commented Jan 17, 2025

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)

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.

@charmander
Copy link
Collaborator

Side note: Node streams’ writable.cork() is a nice way to do this without extra copying and possibly without duplicating the implementation, but I don’t know if that introduces compatibility issues with pg-cloudflare now.

Comment on lines +168 to +174
packets.push(serialize.bind({
portal: query.portal,
statement: query.name,
values: query.values,
binary: query.binary,
valueMapper: valueMapper,
}))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the Flush message when query.rows is present?

@Phara0h
Copy link

Phara0h commented Jan 21, 2025

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

@francois-egner
Copy link

Hello everyone, this bug has existed for a very long time now (since 8.1+) and was last officially reported in October 2024. The fix is available since November 2024. Considering the fact that this bug is an absolute no go for productive applications, I wonder why it is not acted upon with absolute priority. What is the reason? Is more help needed to release the patch?

@cesco69
Copy link
Contributor

cesco69 commented Feb 7, 2025

Hello everyone, this bug has existed for a very long time now (since 8.1+) and was last officially reported in October 2024. The fix is available since November 2024. Considering the fact that this bug is an absolute no go for productive applications, I wonder why it is not acted upon with absolute priority. What is the reason? Is more help needed to release the patch?

Me too, I notice in this project a lot of slowness in releasing new patches/features. How can we help?

Another example is "pg-native don't works on Node 23" ( #3332 ) opened at Oct 17, 2024 fixed/merged but not released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants