Transactional migration (#70) #73
Conversation
merge master
…ry fails, or the node process restarts during the query is running, it will automatically rollback. poolSize: also use `max` as valid config parameter (its the underlying pg-pool param) tests: changed timeouts to work with remote database; unfortunately the speedTest.js doesnt work with my remote database
2 similar comments
|
@phips28 Thanks for this! 🙇 btw, I downgraded bluebird to a dev dependency, which actually introduces a use case that Travis didn't catch since it would only show up with the --production arg or NODE_ENV var. Also, all the commands don't need an extra semicolon, do they? |
Yes thats true, I first added the I removed it |
There was a problem hiding this comment.
Some minor things. I was able to make these changes and get the tests working again. I'd prefer to not merge with all the tests blowing up. Bad optics as they say :).
One more thing. I'd really feel better about this if we created a mock migration to force a failure and then confirm it wasn't committed.
Thanks again
src/contractor.js
Outdated
| @@ -2,6 +2,7 @@ const assert = require('assert'); | |||
| const plans = require('./plans'); | |||
| const migrations = require('./migrations'); | |||
| const schemaVersion = require('../version.json').schema; | |||
| const Promise = require('bluebird'); | |||
src/contractor.js
Outdated
| // use transaction, in case one query fails, it will automatically rollback to avoid inconsistency | ||
| let queryInTransaction = ` | ||
| BEGIN; | ||
| ${plans.create(this.config.schema).join(';')} |
There was a problem hiding this comment.
Add semicolon after the join() to terminate last command
src/contractor.js
Outdated
| let queryInTransaction = ` | ||
| BEGIN; | ||
| ${plans.create(this.config.schema).join(';')} | ||
| ${plans.insertVersion(this.config.schema).replace('$1', `'${schemaVersion}'`)} |
There was a problem hiding this comment.
Add semicolon after this as well to terminate command
src/contractor.js
Outdated
| // use transaction, in case one query fails, it will automatically rollback to avoid inconsistency | ||
| let queryInTransaction = ` | ||
| BEGIN; | ||
| ${migration.commands.join(';')} |
There was a problem hiding this comment.
Add semicolon after the join() to terminate last command
|
|
||
| }); | ||
|
|
||
| it('connection count does not exceed configured pool size with `max`', function(finished){ |
There was a problem hiding this comment.
I'd prefer to move this into a config test (does poolSize == max) instead of duplicating the entire connection pooling test
There was a problem hiding this comment.
added new tests, do you like it that way? should we remove the pooling test?
There was a problem hiding this comment.
Thanks! I'll go ahead and merge and take a stab at the migration failure rolllback test
Issue: #70
Migration: use pg transaction to avoid inconsistency. In case one query fails, or the node process restarts during the query is running, it will automatically rollback.
Param
poolSize: also usemaxas valid config parameter (its the underlying pg-pool param)Tests: changed timeouts to work with my remote database; unfortunately the
speedTest.jsdoesnt work with my remote database 👎 but works on travis 👍