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

Submit queries immediately rather that waiting for ReadyForQuery, when possible #662

Closed
wants to merge 1 commit into from

Conversation

jusou
Copy link

@jusou jusou commented Oct 20, 2014

This pull request causes node-postgres to send queries immediately, unless a copyFrom request has been sent and has not completed.

This fixes the current disastrous state of this library that always waits for query results before submitting the next query, resulting in O(n) rather than O(1) latency for n queries sent without waiting by a client.

API additions:

  • Client.sendImmediately can be set to false to restore the previous behavior
  • Query.blocking can be set to block sending any further query until this query is completed

Internal additions:

  • Client.handshakeDone is set if any ReadyForQuery has been seen
  • Client.sentQueryQuery is the queue of queries that have been sent on the wire

If you would prefer to keep the old behavior by default (not recommended, since it means being broken by default), you can remove the "this.sendImmediately = true;" line.

@brianc
Copy link
Owner

brianc commented Oct 20, 2014

This feature needs to be opt-in, not enabled by default. Regardless of your feelings on the matter, you're incorrect in assuming this is the "proper" behavior. I'll also ask that you edit your pull request text to remove the negativity.

All the tests need to pass as well.

@jusou
Copy link
Author

jusou commented Oct 20, 2014

Sorry, I thought "make test" would run all tests, but it doesn't, and it seems this is somehow broken at the moment.

Looking into fixing it.

@brianc
Copy link
Owner

brianc commented Oct 20, 2014

make test-all should run all the tests - you need a running version of postgres accessible somewhere (generally I use environment variables to connect... PGHOST, PGPASSWORD, etc)

@brianc
Copy link
Owner

brianc commented Oct 20, 2014

I don't think this belongs in node-postgres. I try to keep node-postgres having exact feature parity with the native bindings. There is no way to do this with the native bindings because it isn't supported by libpq (because it doesn't make sense in most cases to do what you want to do). I'll open an issue as a reminder to myself to look into implementing this feature in the future when I have time to pull the 'connection' class out of this library and make it easier to build custom modules on top of the protocol parser. In that case we can have a pg.js module that exposes a simpler interface and includes fancy features like this that can't be shared with libpq and therefore don't belong in node-postgres. See pg-cursor as an example. In the mean time I appreciate your effort, but disagree with your approach and your solution so I'm going to reject this pull request.

@brianc brianc closed this Oct 20, 2014
@jusou
Copy link
Author

jusou commented Oct 20, 2014

I think I fixed the issue, which was due to the fact that the current code sends Sync in response to errors and commandcomplete, which is fixed by sending it unconditionally (btw, I think it's wrong to send Sync this way regardless of this feature). It appears to work now, although I'm having issues running all the tests on my current setup (it seems they are broken when using Unix sockets).

It seems a bad idea to refuse to add essential features just because libpq (which is clearly a badly designed library as we can see there) doesn't support them.

And again, this feature is essential, since not having it will slow applications, potentially making them unusable.

Also, a new user that realizes the way the library is currently implemented is going to rightfully have a terrible opinion of its author, the library and the general state of Node, since this is the #1 thing to get right and the ONLY advantage that using Node provides to an application that only does database calls.

[Note: you can do this kind of thing in other languages, but having built-in promises makes it natural, and means you can do it without changing the API to return promises, which is why nothing else does it]

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.

2 participants