Prepared statements still completely broken

[booyou]

In a recent commit, prepared statements were "fixed" by considering them prepared when CommandComplete is received.

However, this is still wrong, because you should do this when ParseComplete is received rather than CommandComplete.

Your code is currently completely broken in cases where statement preparation succeeds (sending a ParseComplete and creating the statement), but statement execution fails, which results in CommandComplete not being sent.

This was posted as issue #664, but the maintainer decided to censor it for some reason. It is really unfortunate that GitHub allows maintainers to abuse their power like this, and I wonder if there are any other subtle issues being censored.

[brianc]

Warning: if you devolve back into vitriolic and unconstructive conversation I will block you again.

The current implementation is proven to work with test cases - I fail to understand how it's "completely broken." If your statement fails in the parse, bind, or execute phase the list of prepared statements isn't updated with the failed query. Next time you submit the query it will try to prepare it again without caching it until the entire sequence of steps succeeds. Until the query executes successfully to completion its plan wont be considered 'cached' on the client. That's the implementation I have chosen.

If you think you have a better implementation I encourage you to submit a pull request with a test case proving the point. If you'd rather argue and be nasty then I will continue to block and report you to github.

I reserve the right to censor anyone's comments on my repositories if they are being hostile and offensive. You have been completely unacceptable in the way you've approached me and this issue. I censored the comment because the user name is 'fuckyoubrianc' and the contents were full of insults. I censored the previous comment because you made sweeping insults to the node.js community and my work in particular without offering any suggested fix.

This is the last time I will engage with you on this issue or any others without a pull request. I frankly have too many kind people relying on this module to waste any more time dealing with this kind of garbage.

[booyou]

If your statement fails in the parse, bind, or execute phase the list of prepared statements isn't updated with the failed query

And this is wrong...

The statement is created on the server if and only if the server returns ParseComplete in response to Prepare, i.e. if the "parse" phase succeeds.

Whether the bind or execute phases succeeds or not has no effect on whether the named prepared statement has been created on the server.

That's why you need to act in response to ParseComplete and not CommandComplete.

Just read http://www.postgresql.org/docs/9.3/static/protocol-flow.html

In case you don't realize it, the server will return an error if you try to prepare a statement again when one already exists, so you must get this right.

Regarding the so-called negativity, the problem is that this project unfortunately seems to be the only PostgreSQL Node binding existing, exists for more than 3 years, has no disclaimers regarding being experimental, yet has critical issues like this one that will crash your program whenever a statement happens to error (which happen during normal operation in many applications).

[pfrazee]

@booyou can you provide a test case that illustrates this?

[booyou]

Send a correct prepared statement that fails in execution (e.g. an INSERT that violates an unique constraint), then attempt to reissue it again in a way that should succeed by using different paramaters (e.g. an INSERT that does not violate an unique constraint).

Boom! Error: "prepared statement already exists".

[pfrazee]

Test case.

[brianc]

Whether the bind or execute phases succeeds or not has no effect on whether the named prepared statement has been created on the server.

I realize this, but it doesn't hurt to only cache the statement if the entire statement succeeds. This library doesn't expose a way to separate the prepare and execute steps at all, so hiding this from the user is an API decision. I still fail to see how this is critical, catastrophic, or even the wrong approach. Can you create a test case in which the current behavior causes an error or crashes the application? If you can't then we should close this issue.

This library is not experimental. It has indeed been around for 4 years and is used in production by hundreds of companies. I have personally run over 100 million queries through node-postgres in a production environment under heavy load. Bugs are a part of software development. I strive to keep them to a minimum, but the existence of bugs does not render a library experiemental.

[chaoran]

@booyou This is a open-source community. That means if you see something broken, write a test case, and send a pull request with a fix. The author has no responsibly of any sort, as it is written in the license:
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

Pointing fingers and blaming is entirely not helpful at all.

[booyou]

it doesn't hurt to only cache the statement if the entire statement succeeds

No, it hurts...

The server returns an error if you prepare a statement again with the same name...

You need to get this exactly right because you get an error either way if you get it wrong.

I described how to write the test case, but I'm not going to take the time to write it myself, especially because I'm not familiar with node-postgres' conventions for test cases.

[pfrazee]

I described how to write the test case, but I'm not going to take the time to write it myself, especially because I'm not familiar with node-postgres' conventions for test cases.

Then don't expect a fix.

[brianc]

How about an executable gist to reproduce the issue? If it really is a problem I do want to fix it, but I honestly can't reproduce what you're saying...

var pg = require('pg.js')

pg.connect(function(err, client, done) {
  var q = {
    text: 'SELECT $1::int as name',
    values: ['brian'],
    name: 'test'
  }
  client.query(q, function(err, res) {
    console.log(err)
    q.values = [1]
    client.query(q, function(err, res) {
      console.log(err) //null
      console.log(res.rows[0]) //{name: 1}
      done()
    })
  })
})

The above works fine for me.

[booyou]

Are you using the latest version after you committed the fix?

Because before that commit it was wrong the other way.

[booyou]

Your testcase is correct.

I get this running on current master.

{ [error: invalid input syntax for integer: "brian"]
  name: 'error',
  length: 87,
  severity: 'ERROR',
  code: '22P02',
  detail: undefined,
  hint: undefined,
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'numutils.c',
  line: '62',
  routine: 'pg_atoi' }
{ [error: prepared statement "test" already exists]
  name: 'error',
  length: 101,
  severity: 'ERROR',
  code: '42P05',
  detail: undefined,
  hint: undefined,
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'prepare.c',
  line: '466',
  routine: 'StorePreparedStatement' }

/home/user/pers/src/node-postgres/test.js:14
      console.log(res.rows[0]) //{name: 1}
                     ^
TypeError: Cannot read property 'rows' of undefined
    at null.callback (/home/user/pers/src/node-postgres/test.js:14:22)
    at Query.handleError (/home/user/pers/src/node-postgres/lib/query.js:102:17)
    at null.<anonymous> (/home/user/pers/src/node-postgres/lib/client.js:167:26)
    at EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (/home/user/pers/src/node-postgres/lib/connection.js:109:12)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:746:14)
    at Socket.EventEmitter.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:408:10)
    at emitReadable (_stream_readable.js:404:5)

And this with my fix:

{ [error: invalid input syntax for integer: "brian"]
  name: 'error',
  length: 87,
  severity: 'ERROR',
  code: '22P02',
  detail: undefined,
  hint: undefined,
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'numutils.c',
  line: '62',
  routine: 'pg_atoi' }
null
{ name: 1 }

[brianc]

Alright - I'm able to reproduce the error now with the most recent version!

I'll have this fixed within the day today. I do appreciate you pointing out this bug, but your approach was really hostile. If you would have approached it without as much anger and with possibly a failing test case or gist I'd have been able to fix it with much less heartburn for both of us.

I'll add it as a failing test and then fix it in both the javascript & native bindings. The test case is important to prevent regressions & because I'm about to port the native bindings to a new code base and need to be sure it behaves the same in both places.