Skip to content
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.

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

transactions broken with postgres #12

Closed
kubino opened this Issue May 17, 2013 · 7 comments

Comments

Projects
None yet
2 participants
@kubino
Copy link

kubino commented May 17, 2013

Hi I think there is something wrong with transactions at the moment. In knex.js method is called initTransaction, but in base.js in client folder it is startTransaction. I have tried to rename the method name, but then my postgres client is called with different scope -> this points to node.js top level object

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented May 18, 2013

Hey @kubino - you're right, I did some major refactoring of a few pieces before shipping the initial release and didn't realize I didn't have any tests for transactions. I believe I just fixed it up and added some tests, please take a look and let me know if you see any issues now.

@tgriesser tgriesser closed this May 18, 2013

@kubino

This comment has been minimized.

Copy link
Author

kubino commented May 20, 2013

Hi perfect, transactions are now working. Your library looks amazing. I am pretty new to promises, so it's hard to get full picture of your internals for me. However I have different problem now. In Postgres also Schema changes can be run inside the transaction. This is also a good way how to control order of sql command - I want to ensure delete table is called before create table. I am passing now transaction object to dropTable and createTable methods and inside I am instructing to use same connection. Maybe it would be usefull to support it out of the box.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented May 29, 2013

@kubino - If you want to ensure dropTable is called before createTable, you should be able to achieve that with a promise, no transaction necessary.

Knex.Schema.dropTableIfExists('users').then(function () {

   return Knex.Schema.createTable('users', function (t) {
      t.increments('id');
      t.string('first_name');
      t.string('last_name');
   });

}).then(function() {
   console.log('users has been dropped and created');
})
@kubino

This comment has been minimized.

Copy link
Author

kubino commented May 29, 2013

Hi I did tried this, sorry I forgot to mention it before.
It works with mysql but NOT with postgres. However when runned within transaction and same connection is used, everything is just fine. Not sure where is the difference between these two databases. I guess both are opening new connection, and we are just relying here there everything will get processed by database as it was issued by node server. But this was not my case with Postgres. I believe it can work on other machines...

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented May 29, 2013

@kubino Can you paste some example code where it didn't work.

@kubino

This comment has been minimized.

Copy link
Author

kubino commented May 29, 2013

sorry forget it it was my fault, just called a method instead of passing a function reference in then() !!!

Your library is awesome , I was looking hard for some simple but powerful API. Great Job. Thank you for sharing it

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented May 29, 2013

Glad to hear it! Also, you don't need to create individual when.defer() instances or individual error handlers for each a promise, just return a promise to the promise and the first promise becomes the second... and if any of them fail the last promise gets that error... i.e.:

var promises = [];
_.each(Object.keys(tablesDefinition), function(tblName) {
   promises.push(Knex.Schema.createTable(tblName, tablesDefinition[tblName]).then(function() {
      console.log('Created %s!', tblName);
   }));
});
return when.all(promises).then(null, function(e) { 
   console.log('createTableError: ' + e.toString()) 
});

rather than what you had posted before.

elliotf pushed a commit to elliotf/knex that referenced this issue Nov 24, 2014

Merge pull request tgriesser#12 from dgsan/patch-1
The faq doesn't mention that you do need to manually create test databases prior to running the unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.