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

Add support for single row mode #423

Merged
merged 8 commits into from
Sep 30, 2013
Merged

Add support for single row mode #423

merged 8 commits into from
Sep 30, 2013

Conversation

rpedela
Copy link
Contributor

@rpedela rpedela commented Aug 22, 2013

This adds support for single row mode in Postgres 9.2+. It will only enable support if the user uses the event system rather than a callback.

Since there are no interface changes, I am not sure how to update the tests to verify single row mode is actually enabled. I tested it by running a program that repeatedly reads a decent sized table and used pmap to inspect the program's memory usage. And sure enough instead of using 150+ MB, the program only used 20-30 MB when single row mode was enabled.

Please review the code. In particular, I don't think I handled JS->C boolean values correctly.

rpedela added 2 commits August 22, 2013 13:26
…+. This will enable single row mode only when the user wants to stream rows.
@rpedela
Copy link
Contributor Author

rpedela commented Aug 22, 2013

What does Error: Cannot find module '/home/travis/build/brianc/node-postgres/lib/native/../../build/default/binding' mean?

@brianc
Copy link
Owner

brianc commented Aug 22, 2013

Nice. Any idea what the JavaScript memory consumption is? If you're using parameterized queries in JavaScript they should be using a cursor and I would imagine the memory usage would be low there already.

That error means the compiled module object files don't exist. Looks like this file is including the native bindings by force before the make file compiles them

force-native-with-envvar-tests.jsMessage: Cannot find module '/home/travis/build/brianc/node-postgres/lib/native/../../build/default/binding'

@rpedela
Copy link
Contributor Author

rpedela commented Aug 22, 2013

Okay is that my fault? Or the test's fault? The tests pass 100% when I run them locally.

I did not look at JS memory consumption specifically, but memory consumption seems quite high overall. My table is only 12MB on disk according to Postgres. I know there is some inflation from using text and converting to JSON, but 150 MB seems too large to me. The good news is that the memory consumption goes back down after the query completes and v8 performs a garbage collection. Memory consumption is definitely something we need to look into.

@brianc
Copy link
Owner

brianc commented Aug 22, 2013

I think it's the test's fault.

As far as memory usage yeah I think it could be worked on a bit. I don't know how much less it can be reduced in the JavaScript side of things. We're copying to a string once before possibly converting the value to another value (like an int or something) but most of the time it's just reading strings out of buffers. As far as the native bindings are concerned there's a string in the C side of things and then a different string on the JavaScript side of things as V8 copies it out into its own heap, so it might actually be higher there. Not sure. tl;dr - definitely should look at it. I'll check out why that test is failing.

@brianc
Copy link
Owner

brianc commented Sep 2, 2013

Hey @rpedela sorry for taking so long on this. I updated the Makefile to compile the native bindings before running any of the integration tests. Do you think you could merge (or rebase) master into your feature branch & try it again? My changes should hopefully fix the failing tests.

@rpedela
Copy link
Contributor Author

rpedela commented Sep 3, 2013

Sure. I will give it a shot tomorrow or the day after.

@rpedela
Copy link
Contributor Author

rpedela commented Sep 4, 2013

Alright tests passed!

@brianc
Copy link
Owner

brianc commented Sep 9, 2013

Awesome! This is greatness. I am sorry I've taken so long to mess with it - I want to benchmark & make sure it doesn't have negative performance impacts & I've been extremely slammed with work. Generally I do open source on the weekends if I'm too busy during the week but had to go out of town past two weekends.

@rpedela
Copy link
Contributor Author

rpedela commented Sep 9, 2013

No worries. We all have work and sometimes get "slammed".

@brianc
Copy link
Owner

brianc commented Sep 30, 2013

ahhh I just went to benchmark this so I could merge it and I hit a build error....

I have postgres installed via postgres.app at version 9.3.0

Here's the error output:

> pg@2.5.0 install /Users/bmc/src/node-postgres
> node-gyp rebuild || (exit 0)

  CXX(target) Release/obj.target/binding/src/binding.o
../src/binding.cc:662:10: error: use of undeclared identifier 'PGRES_SINGLE_TUPLE'
    case PGRES_SINGLE_TUPLE:
         ^
1 error generated.
make[1]: *** [Release/obj.target/binding/src/binding.o] Error 1
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Users/bmc/local/node@v0.10.12/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:267:23)
gyp ERR! stack     at ChildProcess.EventEmitter.emit (events.js:98:17)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (child_process.js:789:12)
gyp ERR! System Darwin 12.4.0
gyp ERR! command "node" "/Users/bmc/local/node@v0.10.12/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/bmc/src/node-postgres
gyp ERR! node -v v0.10.12
gyp ERR! node-gyp -v v0.10.0
gyp ERR! not ok

Any idea why that's not building for me, but it is building on travis?

@rpedela
Copy link
Contributor Author

rpedela commented Sep 30, 2013

Maybe you need to do a git pull again or update your Postgres headers? Maybe 9.3 changed the name of that constant (be weird if they did)? Or maybe we found a bug in 9.3? Travis received a PGRES_SINGLE_TUPLE compilation error on the 8.x build which I fixed by putting an #ifdef around. You can look at one of my commits.

@rpedela
Copy link
Contributor Author

rpedela commented Sep 30, 2013

Another possibility is that they moved the definition to a different header which needs to be included. Again, it would be strange though.

@brianc
Copy link
Owner

brianc commented Sep 30, 2013

Figured it out - was using the wrong version of pg_config. That's what I get for being lazy & using postgres.app

@brianc
Copy link
Owner

brianc commented Sep 30, 2013

@rpedela I apologize again for taking so long on this.

Good news is the benchmarks look like it's almost no performance impact! I can merge this right away.

As an aside you've been doing a bunch of quality work on here & I'm in desparate need of a co-maintainer. Would you be interested in having a separate dialog about getting added to the repo as a co-maintainer? If you are, please open an issue on the issue tracker so we can discuss things out in the open. If you'd rather not, that's okay too! 😄

brianc added a commit that referenced this pull request Sep 30, 2013
Add support for single row mode
@brianc brianc merged commit 4fcfc66 into brianc:master Sep 30, 2013
@rpedela
Copy link
Contributor Author

rpedela commented Oct 1, 2013

Let me think about it. I greatly appreciate the offer regardless.

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