Cleanly handle missing stream error on COPY operation. Closes #241

[strk]

Rather than killing on uncaught exception, this patch invokes the callback with an appropriate error message when no stream is defined to copy to and from

[strk]

Actually I'm not sure this commit does a good job. Putting an attempt to copy from stdin under testing finds the connection kind of hanging forever. What's the message that should be sent from Query to signal completion ?

[strk]

The problem with my change is that although it succeeds in returning something to the caller, fails to reset the connection back into a useful state:

var pg = require('pg');
pg.connect('tcp://strk@localhost:5432/strk', function(err, c)
{
  console.log('connected');
  c.query('COPY test FROM stdin;', function(e,r) {
    console.log('SURVIVED, error is: ' + e); // gets here
    c.query('COPY test FROM stdin;', function(e,r) {
      console.log('SURVIVED again, error is: ' + e); // but never gets here
    });
  });
});

[anton-kotenko]

It seems that you are doing copy from in wrong way. There are special methods for this - copyTo and copyFrom,
(not query). Example: var stream = c.copyFrom ('COPY TEST FROM stdin'); stream.on('close', function () { console.log('data is loaded'); }); stream.on('error', function (error) { console.log('something bad happens', error); }); //send data from node to database stream.write('first_csv_formatted_chunk of data'); stream.write('second_csv_formatted_chunk of data'); //notify database, that we have sent all data stream.end().

[strk]

Yeah but my main concern here is avoiding an exception, which kills the service I'm running.
Getting a working COPY query is a secondary problem.
I just tested another approach that avoids the throwing, but then doesn't signal the error. Not sure what's the correct way to report an error is in this case.

[strk]

Beside, it would be good to generally avoid throwing exception on unexpected server responses.

[anton-kotenko]

Is this situation form #241?, If it is - problem is that copy operations switches postgres in special subprotocol, in which it does not respond to common messages. If you want to return it normal state you need to send CopyFail
http://www.postgresql.org/docs/9.1/static/protocol-flow.html#PROTOCOL-COPY.
So i propose to

• send CopyFail message to postgres, if stream if missing
• report error in standart way (or though error event, or though first argument of query method callback)

But this situation will appear only if use copy to copy from query from query method

[strk]

Yep, it makes sense.
I'm trying this out, thanks for the pointer.

[strk]

Much better now (yes, it's from #241).
Only problem left now is the COPY TO.
How to handle that case ?
Here's an example testcase to play with:

var pg = require('pg');
pg.connect('tcp://strk@localhost:5432/strk', function(err, c) 
{ 
  console.log('connected');
  c.query('COPY test FROM stdin;', function(e,r) {
    console.log('SURVIVED COPY FROM');
    if ( e ) console.log('error is: ' + e); 
    else console.dir({result: r}); 
    c.query('COPY test FROM stdin;', function(e,r) {
      console.log('SURVIVED COPY FROM again');
      if ( e ) console.log('error is: ' + e); 
      else console.dir({result: r}); 
      c.query('COPY test TO stdin;', function(e,r) {
        console.log('SURVIVED COPY TO');
        if ( e ) console.log('error is: ' + e); 
        else console.dir({result: r}); 
      });
    });
  });
});

And here's the current output.
I like the output for the COPY FROM but the COPY TO may also need some error reporting.

connected
SURVIVED COPY FROM
error is: error: COPY from stdin failed: No source stream defined
SURVIVED COPY FROM again
error is: error: COPY from stdin failed: No source stream defined
SURVIVED COPY TO
{ result: { command: 'COPY', rowCount: 0, oid: NaN, rows: [] } }

[anton-kotenko]

according to postgres documentation:
The frontend cannot abort the transfer (except by closing the connection or issuing a Cancel request), but it can discard unwanted CopyData and CopyDone messages.
So you can try to cancel query (seems to me good) or just ignore data (seems to me bad, as connection will be busy, and database will be busy, wasting cpu time and IO)
But I don't know will it succeed or not, so you can try.

[anton-kotenko]

Also If you try to write defence from incorrect api usage, maybe it's easier to parse query, understand that it's copy to/copy from and report error before actually doing query?

[strk]

I tried an expensive query and verified that 'copyData' even is not fired before the query completes, so backend is still busy, too late to cancel. Parsing the query sounds very error prone to me.

[anton-kotenko]

You can try to cancel query on copyOutResponse event (it is not used now, but it is marker event, that means that "copy to" query was parsed by database and database was switched to data transfer subprotocol)

[strk]

Seems to be out of my reach.
When I try I get:

events.js:68
throw arguments[1]; // Unhandled 'error' event
^
error: invalid frontend message type 0
at p.parseE (/home/src/node/modules/node-postgres/lib/connection.js:497:11)
at p.parseMessage (/home/src/node/modules/node-postgres/lib/connection.js:357:17)
at Socket.p.attachListeners (/home/src/node/modules/node-postgres/lib/connection.js:84:22)

Do you want to try this yourself ?

[anton-kotenko]

Yes, I can try, but it may take few days to find time to do this.

[strk]

For the time being could you merge this pull and tag a new release
if not else to avoid two possible throw from async function ?

The only thing missing is an error message on COPY TO (the code
simply discards the output).

[brianc]

You guys were up late last night! or early. Or not in US timezone. 😄

Will merge this today. Thanks for looking into this. I'm totally with you on not throwing exceptions...exceptions are very not the node way.