Merge branch 'r-1.0-4' into production

DESCRIPTION

@@ -1,7 +1,7 @@
 Encoding: UTF-8
 Package: RPostgres
-Version: 1.0-3
-Date: 2017-12-06
+Version: 1.0-4
+Date: 2017-12-20
 Title: 'Rcpp' Interface to 'PostgreSQL'
 Authors@R: c(
     person("Hadley", "Wickham", role = "aut"),
@@ -16,6 +16,9 @@ Authors@R: c(
 Description: 
     Fully 'DBI'-compliant 'Rcpp'-backed interface to 'PostgreSQL' <https://www.postgresql.org/>,
     an open-source relational database.
+URL: https://github.com/r-dbi/RPostgres
+BugReports: https://github.com/r-dbi/RPostgres/issues
+SystemRequirements: libpq >= 9.0: libpq-dev (deb) or postgresql-devel (rpm)
 License: GPL-2
 LazyLoad: true
 Depends:

NEWS.md

@@ -1,3 +1,12 @@
+# RPostgres 1.0-4 (2017-12-20)
+
+- Only call `PQcancel()` if the query hasn't completed, fixes transactions on Amazon RedShift (#159, @mmuurr).
+- Fix installation on MacOS.
+- Check libpq version in configure script (need at least 9.0).
+- Fix UBSAN warning: reference binding to null pointer (#156).
+- Fix rchk warning: PROTECT internal temporary SEXP objects (#157).
+- Fix severe memory leak when fetching results (#154).
+
 # RPostgres 1.0-3 (2017-12-01)
 
 Initial release, compliant to the DBI specification.

README.md

@@ -9,17 +9,18 @@ RPostgres is an DBI-compliant interface to the postgres database. It's a ground-
 * Automatically cleans up open connections and result sets, ensuring that you
   don't need to worry about leaking connections or memory.
 
-* Is a little faster, saving ~5 ms per query. (For refernce, it takes around 5ms
+* Is a little faster, saving ~5 ms per query. (For reference, it takes around 5ms
   to retrive a 1000 x 25 result set from a local database, so this is 
   decent speed up for smaller queries.)
 
 * A simplified build process that relies on system libpq.
 
 ## Installation
-
-RPostgres isn't available from CRAN yet, but you can get it from github with:
-
 ```R
+# Install the latest RPostgres release from CRAN:
+install.packages("RPostgres")
+
+# Or the the development version from GitHub:
 # install.packages("remotes")
 remotes::install_github("r-dbi/RPostgres")
 ```

configure

@@ -22,18 +22,22 @@ PKG_LIBS_STATIC="-lpq -lssl -lcrypto -lldap"
 if [ $(command -v pkg-config) ]; then
   PKGCONFIG_CFLAGS=$(pkg-config --cflags --silence-errors ${PKG_CONFIG_NAME})
   PKGCONFIG_LIBS=$(pkg-config --libs --silence-errors ${PKG_CONFIG_NAME})
+  PKGCONFIG_MODVERSION=$(pkg-config --modversion --silence-errors ${PKG_CONFIG_NAME})
 
   # MacOS seems to ship a broken libpq.pc file
-  if [[ "$PKGCONFIG_CFLAGS" == *"Internal.sdk"* ]]; then
-    unset PKGCONFIG_CFLAGS
-    unset PKGCONFIG_LIBS
+  if [[ "$OSTYPE" == "darwin"* ]]; then
+    if [[ "$PKGCONFIG_CFLAGS" == *"Internal.sdk"* ]] || [[ -z "$PKGCONFIG_CFLAGS" ]]; then
+      unset PKGCONFIG_CFLAGS
+      unset PKGCONFIG_LIBS
+    fi
   fi
 fi
 
 # pg_config values (if available)
 if [ $(command -v pg_config) ]; then
   PG_INC_DIR=$(pg_config --includedir)
   PG_LIB_DIR=$(pg_config --libdir)
+  PG_VERSION=$(pg_config --version)
 fi
 
 # Note that cflags may be empty in case of success
@@ -42,11 +46,15 @@ if [ "$INCLUDE_DIR" ] || [ "$LIB_DIR" ]; then
   PKG_CFLAGS="-I$INCLUDE_DIR $PKG_CFLAGS"
   PKG_LIBS="-L$LIB_DIR $PKG_LIBS"
 elif [ "$PKGCONFIG_CFLAGS" ] || [ "$PKGCONFIG_LIBS" ]; then
-  echo "Using pkg-config cflags and libs!"
+  echo "Found pkg-config cflags and libs ($PKG_CONFIG_NAME $PKGCONFIG_MODVERSION)!"
   PKG_CFLAGS=${PKGCONFIG_CFLAGS}
   PKG_LIBS=${PKGCONFIG_LIBS}
 elif [ "$PG_INC_DIR" ] || [ "$PG_LIB_DIR" ]; then
-  echo "Using pg_config includedir and libdir!"
+  echo "Found pg_config includedir and libdir ($PG_VERSION)!"
+  if [[ "$PG_VERSION" == "PostgreSQL 8"* ]]; then
+    echo "This version of libpq is too old! We need at least 9.0!"
+    exit 1
+  fi
   PKG_CFLAGS="-I${PG_INC_DIR}"
   PKG_LIBS="-L${PG_LIB_DIR} ${PKG_LIBS}"
 elif [[ "$OSTYPE" == "darwin"* ]]; then
@@ -94,11 +102,10 @@ fi
 
 # Write to Makevars
 sed -e "s|@cflags@|$PKG_CFLAGS|" -e "s|@libs@|$PKG_LIBS|" src/Makevars.in > src/Makevars.new
-if [ ! -f src/Makevars.new ] || (which diff > /dev/null && ! diff -q src/Makevars src/Makevars.new); then
-  mv src/Makevars.new src/Makevars
-else
-  rm src/Makevars.new
+if [ ! -f src/Makevars ] || (which diff > /dev/null && ! diff -q src/Makevars src/Makevars.new); then
+  cp -f src/Makevars.new src/Makevars
 fi
+rm -f src/Makevars.new
 
 # Success
 exit 0

cran-comments.md

@@ -1,17 +1,13 @@
-Resubmission upon CRAN's request.
-
-- Version 1.0-3 now runs tests and examples if a default PostgreSQL connection is available (via env vars listed in <https://www.postgresql.org/docs/9.6/static/libpq-envars.html>), and silently succeeds if not. Version 1.0-2 did the same with testthat 2.0.0 which is not on CRAN yet, sorry for the omission.
+Fix for UBSAN and rchk erros, a memory leak, and compatibility problems with Amazon RedShift.
 
 ## Test environments
-* local ubuntu install, R 3.4.2
-* ubuntu 14.04 (on travis-ci), R 3.4.2, devel, oldrel, 3.2
-* windows (on appveyor), R 3.4.2
+* local ubuntu install, R 3.4.3
+* ubuntu 14.04 (on travis-ci), R 3.4.3, devel, oldrel, 3.2
+* windows (on appveyor), R 3.4.3
 * win-builder (devel and release)
 
 ## R CMD check results
 
-0 errors | 0 warnings | 2 notes
-
-* This is a new release.
+0 errors | 0 warnings | 1 note
 
 * We link libpq (and libcrypto) statically on Windows, hence the size.

src/DbColumn.cpp

@@ -72,12 +72,13 @@ void DbColumn::warn_type_conflicts(const String& name) const {
 
 DbColumn::operator SEXP() const {
   DATA_TYPE dt = get_last_storage()->get_data_type();
-  SEXP ret = DbColumnStorage::allocate(n, dt);
+  SEXP ret = PROTECT(DbColumnStorage::allocate(n, dt));
   int pos = 0;
   for (size_t k = 0; k < storage.size(); ++k) {
     const DbColumnStorage& current = storage[k];
     pos += current.copy_to(ret, dt, pos);
   }
+  UNPROTECT(1);
   return ret;
 }
 

src/DbColumnStorage.cpp

@@ -38,9 +38,10 @@ SEXP DbColumnStorage::allocate(const R_xlen_t length, DATA_TYPE dt) {
   SEXPTYPE type = sexptype_from_datatype(dt);
   RObject class_ = class_from_datatype(dt);
 
-  SEXP ret = Rf_allocVector(type, length);
+  SEXP ret = PROTECT(Rf_allocVector(type, length));
   if (!Rf_isNull(class_)) Rf_setAttrib(ret, R_ClassSymbol, class_);
   set_attribs_from_datatype(ret, dt);
+  UNPROTECT(1);
   return ret;
 }
 
@@ -209,7 +210,8 @@ Rcpp::RObject DbColumnStorage::class_from_datatype(DATA_TYPE dt) {
 void DbColumnStorage::set_attribs_from_datatype(SEXP x, DATA_TYPE dt) {
   switch (dt) {
   case DT_TIME:
-    Rf_setAttrib(x, CharacterVector::create("units"), CharacterVector::create("secs"));
+    Rf_setAttrib(x, PROTECT(CharacterVector::create("units")), PROTECT(CharacterVector::create("secs")));
+    UNPROTECT(2);
     break;
 
   default:

src/DbConnection.cpp

@@ -1,6 +1,7 @@
 #include "pch.h"
 #include "DbConnection.h"
 #include "encode.h"
+#include "DbResult.h"
 
 
 DbConnection::DbConnection(std::vector<std::string> keys, std::vector<std::string> values) :
@@ -44,34 +45,45 @@ PGconn* DbConnection::conn() {
 }
 
 void DbConnection::set_current_result(const DbResult* pResult) {
-  // Cancels previous query, if needed.
+  // same result pointer, nothing to do.
   if (pResult == pCurrentResult_)
     return;
 
+  // try to clean up remnants of any previous queries.
+  // (even if (the new) pResult is NULL, we should try to reset the back-end.)
   if (pCurrentResult_ != NULL) {
-    if (pResult != NULL)
-      warning("Cancelling previous query");
+    if (pResult != NULL) {
+      warning("Closing open result set, cancelling previous query");
+    }
 
     cleanup_query();
   }
+
   pCurrentResult_ = pResult;
 }
 
+
+/**
+ * Documentation for canceling queries:
+ * https://www.postgresql.org/docs/9.6/static/libpq-cancel.html
+ **/
 void DbConnection::cancel_query() {
   check_connection();
 
-  // Cancel running query
+  // first allocate a 'cancel command' data structure.
+  // should only return NULL if either:
+  //  * the connection is NULL or
+  //  * the connection is invalid.
   PGcancel* cancel = PQgetCancel(pConn_);
-  if (cancel == NULL) {
-    warning("Failed to cancel running query");
-    return;
-  }
+  if (cancel == NULL) stop("Connection error detected via PQgetCancel()");
 
+  // PQcancel() actually issues the cancel command to the backend.
   char errbuf[256];
   if (!PQcancel(cancel, errbuf, sizeof(errbuf))) {
     warning(errbuf);
   }
 
+  // free up the data structure allocated by PQgetCancel().
   PQfreeCancel(cancel);
 }
 
@@ -217,6 +229,8 @@ void DbConnection::conn_stop(PGconn* conn, const char* msg) {
 }
 
 void DbConnection::cleanup_query() {
-  cancel_query();
+  if(pCurrentResult_ != NULL && !(pCurrentResult_->complete())) {
+    cancel_query();
+  }
   finish_query();
 }

src/DbResult.cpp

@@ -50,8 +50,8 @@ int DbResult::n_rows_fetched() {
   return impl->n_rows_fetched();
 }
 
-bool DbResult::complete() {
-  return impl->complete();
+bool DbResult::complete() const {
+  return (impl == NULL) || impl->complete();
 }
 
 List DbResult::get_column_info() {

src/DbResult.h

@@ -25,7 +25,7 @@ class DbResult : boost::noncopyable {
   ~DbResult();
 
 public:
-  bool complete();
+  bool complete() const;
   bool active() const;
   int n_rows_fetched();
   int n_rows_affected();

src/PqResultImpl.cpp

@@ -34,8 +34,9 @@ pRes_(NULL)
 
 PqResultImpl::~PqResultImpl() {
   try {
-    PQclear(pSpec_);
+    if (pSpec_) PQclear(pSpec_);
   } catch (...) {}
+  if (pRes_) PQclear(pRes_);
 }
 
 
@@ -169,7 +170,7 @@ void PqResultImpl::init(bool params_have_rows) {
 
 // Publics /////////////////////////////////////////////////////////////////////
 
-bool PqResultImpl::complete() {
+bool PqResultImpl::complete() const {
   return complete_;
 }
 
@@ -286,8 +287,13 @@ bool PqResultImpl::bind_row() {
     }
   }
 
-  if (!PQsendQueryPrepared(pConn_, "", cache.nparams_, &c_params[0],
-                           &lengths[0], &formats[0], 0))
+  // Pointer to first element of empty vector is undefined behavior!
+  int success = cache.nparams_ ?
+    PQsendQueryPrepared(pConn_, "", cache.nparams_, &c_params[0],
+                      &lengths[0], &formats[0], 0) :
+    PQsendQueryPrepared(pConn_, "", 0, NULL, NULL, NULL, 0);
+
+  if (!success)
     conn_stop("Failed to send query");
 
   if (!PQsetSingleRowMode(pConn_))
@@ -333,6 +339,7 @@ void PqResultImpl::step() {
 bool PqResultImpl::step_run() {
   LOG_VERBOSE;
 
+  if (pRes_) PQclear(pRes_);
   pRes_ = PQgetResult(pConn_);
 
   // We're done, but we need to call PQgetResult until it returns NULL
@@ -345,7 +352,6 @@ bool PqResultImpl::step_run() {
   }
 
   if (pRes_ == NULL) {
-    PQclear(pRes_);
     stop("No active query");
   }
 
@@ -355,6 +361,7 @@ bool PqResultImpl::step_run() {
   case PGRES_FATAL_ERROR:
     {
       PQclear(pRes_);
+      pRes_ = NULL;
       conn_stop("Failed to fetch row");
       return false;
     }

src/PqResultImpl.h

@@ -47,7 +47,7 @@ class PqResultImpl : boost::noncopyable, public PqResultSource {
   void init(bool params_have_rows);
 
 public:
-  bool complete();
+  bool complete() const;
   int n_rows_fetched();
   int n_rows_affected();
   void bind(const List& params);

tests/testthat/test-dbConnect.R

@@ -12,7 +12,11 @@ test_that("warn if previous result set is invalidated", {
 
   rs1 <- dbSendQuery(con, "SELECT 1 + 1")
 
-  expect_warning(rs2 <- dbSendQuery(con, "SELECT 1 + 1"), "Cancelling previous query")
+  expect_warning(
+    rs2 <- dbSendQuery(con, "SELECT 1 + 1"),
+    "Closing open result set, cancelling previous query",
+    fixed = TRUE
+  )
   expect_false(dbIsValid(rs1))
 
   dbClearResult(rs2)