only PQcancel if DbResult::complete() is false

[mmuurr]

The test at line 220:

if(pCurrentResult_ != NULL && !(const_cast<DbResult*>(pCurrentResult_))->complete())

... is a little ugly, though the const_cast was needed since pCurrentResult_ is declared as a const *DbResult but the method implementations don't expect a const this.
Rather than re-write a lot of the other code, I thought this was the path of least resistance. (Especially since complete() doesn't change anything in the DbResult, only gets a value; i.e. the const_cast is safe here.)

I also cleaned up some of the logic in set_current_result, since if we're setting a new result, regardless of that new result's value (i.e. even if it's NULL) we should try to clean-up the previous result.

Finally, I made a few changes to the cancel_query() function itself, as the comments didn't exactly reflect the documented pattern described here. (I also placed a link to those docs in the comments for cancel_query().)

So far in my own testing, this change works and still behaves nicely if folks try to issue a new query prior to calling dbCleanResult().

[krlmlr]

Thanks! I think ultimately cleanup_query() should be part of the PqResult class, but I can refactor after merging. Would you like to do another round?

[krlmlr]

I think DbResult::complete() can be made const, by declaring bool complete() const in the header file. I don't mind addong const to all other accessors as part of this.

[krlmlr]

We're already calling check_connection() in line 69?

[krlmlr]

Would you like to do another round? I'd like to release RPostgres soon, and I can do it myself if you don't have the time. Thanks!

[mmuurr]

@krlmlr I cleaned up that ugly const_cast. There's definitely a bunch of other places that can use some const love, but I'm pretty time-committed for the next two weeks. Happy to do a larger pass and help with const refactoring once my schedule slows down a bit at the start of the new year. In the meantime, this last commit tidies things up a bit w.r.t. complete() and keeps Amazon Redshift transactions happy :-)

Also, I'm not 100% sure cleanup_query() should be moved to the result classes, only because while queries can be started asynchronously, the connection can only handle fetching from one at a time, and libpq's cancel architecture doesn't differentiate between the (possibly many) prepared results ... it only tries to cancel the very last command sent to the connection. I.e. it is a connection-specific procedure.
On the other hand, since RPostgres only lets the connection object deal with a single DbResult at a time, I can see your reasoning for wanting pushing it to the result logic.
Something to stew on a bit more, and I definitely didn't want to make any sort of changes that large with this PR, so I punted.

Cheers!

[krlmlr]

Thanks. This segfaults on my machine, and on Travis CI: https://travis-ci.org/r-dbi/RPostgres/jobs/317506005#L498. Do you have time to take a quick glance?

[krlmlr]

Segfault is triggered by devtools::test().

[krlmlr]

We need to check for impl being non-NULL in complete(). Also, I'm seeing many warnings when running the tests, maybe the complete_ flag isn't set for certain kinds of queries?

[krlmlr]

Thanks! Fixed the remaining problems and submitted to CRAN.