Skip to content

521 test robustness pgbouncer #522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 30, 2018
Merged

521 test robustness pgbouncer #522

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6eeb949
This is a bash script #521
Jul 19, 2018
20d1db7
Helper to reset pgbouncer connections #521
Jul 19, 2018
5b65252
Ensure no connection pgbouncer -> test_db #521
Jul 19, 2018
7277143
Merge remote-tracking branch 'origin/master' into 521-test-robustness…
Jul 25, 2018
bf1d5bf
Early return instead of if/else
Jul 30, 2018
2afe845
More descriptive comment about the intent of the code
Jul 30, 2018
942b007
Please jshint
Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion test/acceptance/batch/batch-limits.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var BatchTestClient = require('../../support/batch-test-client');
var JobStatus = require('../../../batch/job_status');
var redisUtils = require('../../support/redis_utils');
var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() });
const db_utils = require('../../support/db_utils');

describe('batch query statement_timeout limit', function() {

Expand All @@ -14,13 +15,14 @@ describe('batch query statement_timeout limit', function() {
global.settings.batch_query_timeout = 15000;
metadataBackend.redisCmd(5, 'HMSET', ['limits:batch:vizzuality', 'timeout', 100], done);
});

before(db_utils.resetPgBouncerConnections);
after(function(done) {
global.settings.batch_query_timeout = this.batchQueryTimeout;
redisUtils.clean('limits:batch:*', function() {
this.batchTestClient.drain(done);
}.bind(this));
});
after(db_utils.resetPgBouncerConnections);

function jobPayload(query) {
return {
Expand Down
4 changes: 4 additions & 0 deletions test/acceptance/export/timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ require('../../support/assert');

var assert = require('assert');
var querystring = require('querystring');
const db_utils = require('../../support/db_utils');

describe('timeout', function () {
describe('export database', function () {
before(db_utils.resetPgBouncerConnections);
after(db_utils.resetPgBouncerConnections);

const databaseTimeoutQuery = `
select
ST_SetSRID(ST_Point(0, 0), 4326) as the_geom,
Expand Down
1 change: 1 addition & 0 deletions test/prepare_db.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ if test x"$PREPARE_PGSQL" = xyes; then

echo "preparing postgres..."
echo "PostgreSQL server version: `psql -A -t -c 'select version()'`"
echo "PAUSE; RESUME;" | psql -p 6432 pgbouncer # make sure there are no connections pgbouncer -> test_db
dropdb ${TEST_DB} # 2> /dev/null # error expected if doesn't exist, but not otherwise
createdb -Ttemplate_postgis -EUTF8 ${TEST_DB} || die "Could not create test database"
psql -c 'CREATE EXTENSION IF NOT EXISTS "uuid-ossp";' ${TEST_DB}
Expand Down
2 changes: 1 addition & 1 deletion test/run_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/bash

# To make output dates deterministic
export TZ='Europe/Rome'
Expand Down
37 changes: 37 additions & 0 deletions test/support/db_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';

const { Client } = require('pg');

const dbConfig = {
db_user: process.env.PGUSER || 'postgres',
db_host: global.settings.db_host,
db_port: global.settings.db_port,
db_batch_port: global.settings.db_batch_port
};

module.exports.resetPgBouncerConnections = function (callback) {
// We assume there's no pgbouncer if db_port === db_batch_port
if (dbConfig.db_port === dbConfig.db_batch_port) {
return callback();
}

const client = new Client({
database: 'pgbouncer',
user: dbConfig.db_user,
host: dbConfig.db_host,
port: dbConfig.db_port
});

// We just chain a PAUSE followed by a RESUME
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ei, what is the objective of pause - resume?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pgbouncer.github.io/usage.html#admin-console

PAUSE [db];
PgBouncer tries to disconnect from all servers, first waiting for all queries to complete. The command will not return before all queries are finished. To be used at the time of database restart.

RESUME [db];
Resume work from previous PAUSE or SUSPEND command.

Wait for all the running queries to finish and let the new ones reconnect. This way you pick the changed session parameters such as set statement_timeout. Sames happens in production, BTW, but there the connections are refreshed usually more often.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks! :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about // We just chain a PAUSE followed by a RESUME to reset internal pool connections of PgBouncer?

client.connect();
client.query('PAUSE', (err, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, follow Node.js conventions:

client.query('PAUSE', (err, res) => {
  if (err) {
    return callback(err);
  }
  client.query('RESUME', (err, res) => ...);
}

If you inmediatly return after calling the callback, there is no need to wrap following sentences into a else statement.

if (err) {
return callback(err);
} else {
client.query('RESUME', (err, res) => {
client.end();
return callback(err);
});
}
});
}