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

Bind Buffer Variables as binary values (with Native implementation also) #447

Merged
merged 4 commits into from
Oct 3, 2013

Conversation

eugeneware
Copy link
Contributor

Avoids having to encode blobs in queries.

This is a reopen of issue #92 by @nraynaud

I've updated the original javascript-only PR to work with the current version of node-postgres, as well as implement changes in the native driver as well.

The native approach was to simply encode any buffer into the bytea hex format as suggested at http://www.postgresql.org/docs/9.0/static/datatype-binary.html

@rpedela
Copy link
Contributor

rpedela commented Sep 19, 2013

Would this work for blobs too using the lo extension?

@eugeneware
Copy link
Contributor Author

I don't believe so. This only will escape buffers that are added into SQL statements, and in particular, prepared statements.

From my knowledge of large objects, it looks like you need to use the large object API to create an object ID which then gets attached to a query.

@brianc
Copy link
Owner

brianc commented Sep 21, 2013

This is pretty radical! Thanks a ton. Question - will this break backwards compatibility in any way? It previously just "didn't work" if you supplied a buffer parameter right? So this is just a new feature and not breaking anything? I'm cool either way, just need to know to bump major or minor!

@eugeneware
Copy link
Contributor Author

Yes. This is a new feature. Passing a buffer to the native driver would throw an error. And to the javascript driver would coerce to a string and also not do the right thing.

@BobCochran
Copy link

I'm new to node.js and node-postgres and don't have much experience with Github. With that said I have a need to process photo images which need to be stored on Postgres as the bytea datatype. I also have a need to fairly rapid prototyping of a web page for someone, which is where node comes in. So I'd like to try out eugeneware's code if that will help all 3 of us. How do I go about doing this?

@eugeneware
Copy link
Contributor Author

Hi @BobCochran, The easiest thing would be to work off my fork at https://github.com/eugeneware/node-postgres/tree/buffer-params

Make sure you use the buffer-params fork.

NB: The other way to insert binary without needing this fix is just to do insert statements, but do the hex conversion yourself.

Ie:

INSERT INTO mytable (blobvalue) VALUE ('\xDEADBEAF');

If you do a buffer.toString('hex') and put a \x in front of it, you should be right.

@BobCochran
Copy link

Hi @eugeneware, thank you very much for your help. I've cloned your repository and will work with it. did a 'git checkout buffer-params' within the clone of your repository, I hope I did this correctly. I will let you know my results. I have some database experience, mostly MySQL and SQLite, with a tiny bit of Oracle, so I'm learning as I go with this project. I also forked Brian's repository because I wanted to add documentation content to the README.md.

@brianc
Copy link
Owner

brianc commented Sep 30, 2013

Hmmm...the C bindings aren't building in node v0.8.x. Once that's fixed I'll happily merge this in to the main line here. Much appreciate this feature. And thanks for your answer too...I don't think this requires a major version bump because it's a new feature & doesn't break backwards compatibility.

Two options for getting the compilation fixed.

  1. Figure out how to get it to build in node v0.8.x, the build error is here

  2. Do a compile-time check of the node version. If the version is below v0.10.x throw an exception saying the feature isnt supported in native bindings below version v.0.10.x. This option is a bit easier and a bit uglier. I'm okay with that since v0.8.x is still supported, but can miss out on a new feature now and again if it causes problems.

How does that sounds to you @eugeneware

@BobCochran
Copy link

Hi @eugeneware I am not sure how to build the code I cloned. I'm running node V0.10.19 on a CentOS version 6.4 x86_64 host. I went into the node-postgres directory and typed `'make' and it seemed to build without a problem. However, I don't see a directory named 'pg' inside the node_modules directory. I assume I've done something wrong. Can you please advise me on to how to build your fork correctly? Please excuse my newness to this. Also, I'm confused on one point: how do I use your code for e.g. an image that will be inserted into a bytea field in the destination table? Thanks for any help you can provide.

@rpedela
Copy link
Contributor

rpedela commented Oct 1, 2013

For the node module inclusion: http://howtonode.org/managing-module-dependencies . You can also require() the root directory of the repo after building it.

@BobCochran
Copy link

@rpedela Thanks for this, I spent the day thinking about it since I was in the field and couldn't experiment. I'm still a little confused but I have a glimmer of what you are pointing me to. Do I need to merge @eugeneware's changes into a clone of Brian's work and then build the module -- perhaps playing with the package.json as the link suggests may be needed? When I have some time I'll compare eugeneware's work to Brian's build tree. Perhaps I need to merge the two or, as you suggest, require() brain's repo.

@rpedela
Copy link
Contributor

rpedela commented Oct 2, 2013

// command-line
git clone https://github.com/eugeneware/node-postgres.git
git checkout -b buffer-params
make

// in Node.js code
var pg = require("/path/to/repo");

@BobCochran
Copy link

@eugeneware, thank you very much! I'll try that.

On Tue, Oct 1, 2013 at 8:46 PM, Ryan Pedela notifications@github.comwrote:

// command-line
git clone https://github.com/eugeneware/node-postgres.git
git checkout -b buffer-params
make

// in Node.js code
var pg = require("/path/to/repo");
...


Reply to this email directly or view it on GitHubhttps://github.com//pull/447#issuecomment-25505219
.

@BobCochran
Copy link

@rpedela I want to apologize for confusing your help with eugeneware's. When I first say your command-line suggestion, it was while I was logged into Gmail, and Gmail had rolled up the new email into a conversational "thread" that made it look like eugeneware was replying to my questions. I'm sorry.

Your command-line and "in Node.js code" suggestions were just the ticket, I followed your advice and at last the light bulb went on for me. Thanks a lot! This is very much appreciated! Tomorrow (Eastern Time) I will start testing and playing.

@rpedela
Copy link
Contributor

rpedela commented Oct 2, 2013

No worries. Good luck!

@eugeneware
Copy link
Contributor Author

Hi @brianc

I've made the fixes for 0.8. Just a casting issue.

Should be good for 0.10 and 0.8 now.

@brianc
Copy link
Owner

brianc commented Oct 3, 2013

badass! merging & pushing new minor version. 💃 👍

brianc added a commit that referenced this pull request Oct 3, 2013
Bind Buffer Variables as binary values (with Native implementation also)
@brianc brianc merged commit aea984f into brianc:master Oct 3, 2013
Joris-van-der-Wel added a commit to Joris-van-der-Wel/node-pg-large-object that referenced this pull request Oct 2, 2014
…inding a variable using a Buffer.

Version 2.7.0 should contain brianc/node-postgres#447
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.

None yet

4 participants