New option to remove client from pool after error happens

This help to avoid issues with transactions.

Closes #241

NEWS.md

@@ -1,6 +1,11 @@
 1.24.0 - 2015-mm-dd
 -------------------
 
+New features:
+
+ * New configuration option `db_pool_destroy_client_on_error`: client is removed from pool after error happens
+   This help to avoid issues with transactions (#241)
+
 Announcements:
 
  * Upgrades cartodb-psql to [0.6.0](https://github.com/CartoDB/node-cartodb-psql/releases/tag/0.6.0)

app/controllers/app.js

@@ -66,6 +66,8 @@ var cdbReq = new CdbRequest();
 // Set default configuration 
 global.settings.db_pubuser = global.settings.db_pubuser || "publicuser";
 global.settings.bufferedRows = global.settings.bufferedRows || 1000;
+global.settings.db_pool_destroy_client_on_error = global.settings.hasOwnProperty('db_pool_destroy_client_on_error') ?
+    global.settings.db_pool_destroy_client_on_error : true;
 
 var tableCache = LRU({
   // store no more than these many items in the cache
@@ -358,7 +360,7 @@ function handleQuery(req, res) {
 
                 checkAborted('queryExplain');
 
-                pg = new PSQL(dbopts);
+                pg = new PSQL(dbopts, {}, { destroyOnError: global.settings.db_pool_destroy_client_on_error });
                 // get all the tables from Cache or SQL
                 tableCacheItem = tableCache.get(sql_md5);
                 if (tableCacheItem) {

app/models/formats/ogr.js

@@ -78,7 +78,7 @@ OgrFormat.prototype.toOGR = function(options, out_format, out_filename, callback
 
     function fetchColumns() {
       var colsql = 'SELECT * FROM (' + sql + ') as _cartodbsqlapi LIMIT 0';
-      pg = new PSQL(dbopts);
+      pg = new PSQL(dbopts, {}, { destroyOnError: global.settings.db_pool_destroy_client_on_error });
       pg.query(colsql, this);
     },
     // jshint maxcomplexity:9

app/models/formats/pg.js

@@ -119,7 +119,7 @@ PostgresFormat.prototype.sendResponse = function(opts, callback) {
 
   this.start_time = Date.now();
 
-  this.client = new PSQL(opts.dbopts);
+  this.client = new PSQL(opts.dbopts, {}, { destroyOnError: global.settings.db_pool_destroy_client_on_error });
   this.client.eventedQuery(sql, function(err, query, queryCanceller) {
       that.queryCanceller = queryCanceller;
       if (err) {

config/environments/development.js.example

@@ -36,6 +36,8 @@ module.exports.db_pool_size = 500;
 module.exports.db_pool_idleTimeout = 30000;
 // Milliseconds between idle client checking
 module.exports.db_pool_reapInterval = 1000;
+// Whether a client must be removed from the pool after an error happened in the query or not
+module.exports.db_pool_destroy_client_on_error = true;
 // max number of bytes for a row, when exceeded the query will throw an error
 //module.exports.db_max_row_size = 10 * 1024 * 1024;
 // allows to use an object to connect with node-postgres instead of a connection string

config/environments/production.js.example

@@ -37,6 +37,8 @@ module.exports.db_pool_size = 500;
 module.exports.db_pool_idleTimeout = 30000;
 // Milliseconds between idle client checking
 module.exports.db_pool_reapInterval = 1000;
+// Whether a client must be removed from the pool after an error happened in the query or not
+module.exports.db_pool_destroy_client_on_error = true;
 // max number of bytes for a row, when exceeded the query will throw an error
 //module.exports.db_max_row_size = 10 * 1024 * 1024;
 // allows to use an object to connect with node-postgres instead of a connection string

config/environments/staging.js.example

@@ -37,6 +37,8 @@ module.exports.db_pool_size = 500;
 module.exports.db_pool_idleTimeout = 30000;
 // Milliseconds between idle client checking
 module.exports.db_pool_reapInterval = 1000;
+// Whether a client must be removed from the pool after an error happened in the query or not
+module.exports.db_pool_destroy_client_on_error = true;
 // max number of bytes for a row, when exceeded the query will throw an error
 //module.exports.db_max_row_size = 10 * 1024 * 1024;
 // allows to use an object to connect with node-postgres instead of a connection string

config/environments/test.js.example

@@ -34,6 +34,8 @@ module.exports.db_pool_size = 500;
 module.exports.db_pool_idleTimeout = 30000;
 // Milliseconds between idle client checking
 module.exports.db_pool_reapInterval = 1000;
+// Whether a client must be removed from the pool after an error happened in the query or not
+module.exports.db_pool_destroy_client_on_error = true;
 // max number of bytes for a row, when exceeded the query will throw an error
 //module.exports.db_max_row_size = 10 * 1024 * 1024;
 // allows to use an object to connect with node-postgres instead of a connection string

npm-shrinkwrap.json

@@ -1,6 +1,6 @@
 {
   "name": "cartodb_sql_api",
-  "version": "1.23.1",
+  "version": "1.24.0",
   "dependencies": {
     "cartodb-psql": {
       "version": "0.6.0",

package.json

@@ -5,7 +5,7 @@
     "keywords": [
       "cartodb"
     ],
-    "version": "1.23.1",
+    "version": "1.24.0",
     "repository": {
         "type": "git",
         "url": "git://github.com/CartoDB/CartoDB-SQL-API.git"
@@ -31,6 +31,7 @@
     },
     "devDependencies": {
         "redis": "0.7.1",
+        "request": "~2.60.0",
         "shapefile": "0.3.0",
         "mocha": "~1.21.4",
         "jshint": "~2.6.0",

test/acceptance/transaction.js

@@ -0,0 +1,89 @@
+require('../helper');
+
+var assert = require('../support/assert');
+var qs = require('querystring');
+var request = require('request');
+
+describe('transaction', function() {
+
+    var SERVER_PORT = 5554;
+
+    var server;
+    beforeEach(function(done) {
+        server = require(global.settings.app_root + '/app/controllers/app')();
+        server.listen(SERVER_PORT, '127.0.0.1', done);
+    });
+
+    afterEach(function(done) {
+        global.settings.db_pool_destroy_client_on_error = true;
+        server.close(done);
+    });
+
+    var sqlRequest = request.defaults({
+        headers: { host: 'vizzuality.localhost' }
+    });
+
+    function createRequest(query) {
+        return 'http://127.0.0.1:' + SERVER_PORT + '/api/v1/sql?' + qs.stringify({ q: query });
+    }
+
+
+    var destroyOnErrorScenarios = [
+        {
+            destroyOnError: true,
+            assertFn: function (done) {
+                return function (err, response, body) {
+                    assert.ok(!err);
+                    assert.equal(response.statusCode, 200);
+
+                    var parsedBody = JSON.parse(body);
+                    assert.ok(parsedBody);
+                    assert.deepEqual(parsedBody.rows, [{ foo: 1 }]);
+
+                    done();
+                };
+            }
+        },
+        {
+            destroyOnError: false,
+            assertFn: function(done) {
+                return function(err, response, body) {
+                    assert.ok(!err);
+                    assert.equal(response.statusCode, 400);
+
+                    var parsedBody = JSON.parse(body);
+                    assert.ok(parsedBody);
+                    assert.deepEqual(
+                        parsedBody.error,
+                        ['current transaction is aborted, commands ignored until end of transaction block']
+                    );
+
+                    // do extra request to clean up scenario
+                    global.settings.db_pool_destroy_client_on_error = true;
+                    sqlRequest(createRequest('select 1'), done);
+                };
+            }
+        }
+    ];
+
+    var errorQuery = 'BEGIN; PREPARE _pstm AS select error; EXECUTE _pstm; COMMIT;';
+
+    destroyOnErrorScenarios.forEach(function(scenario) {
+        var shouldOrShouldNot = scenario.destroyOnError ? ' NOT ' : ' ';
+        it('should' + shouldOrShouldNot + 'fail to second request after error in transaction', function(done) {
+            global.settings.db_pool_destroy_client_on_error = scenario.destroyOnError;
+
+            sqlRequest(createRequest(errorQuery), function(err, response, body) {
+                assert.ok(!err);
+                assert.equal(response.statusCode, 400);
+
+                var parsedBody = JSON.parse(body);
+                assert.ok(parsedBody);
+                assert.deepEqual(parsedBody, { error: ['column "error" does not exist'] });
+
+                sqlRequest(createRequest('select 1 as foo'), scenario.assertFn(done));
+            });
+        });
+    });
+
+});