Adding support for transaction savepoint

[SohumB]

var knex = require('knex').initialize({..});

var q1 = knex('books').insert({ name: 'Canterbury Tales' });
var q2 = knex('books').insert({ name: 'Moby Dick' });

knex.transaction(function(t) {
  return q1.transacting(t).then(function(a) {
    return knex.transaction(function(t2) {
      return q2.transacting(t2).then(function(b) {
      });
    });
  });
}).then(function() { console.log('fine'); process.exit(0); },
        function() { console.log('problem'); process.exit(1); });

This script succeeds under normal operation on a simple books schema. But under pool: { max: 1, min: 1 }, it hangs.

[tgriesser]

Is there a reason you're executing nested transactions in this way? I don't believe this behavior should be supported, instead you can do:

knex.transaction(function(t) {
  return q1.transacting(t);
}).then(function(a) {
  return knex.transaction(function(t2) {
    return q2.transacting(t2).then(function(b) {
      //...
    });
  });  
}).then(function() {
  console.log('fine'); process.exit(0);
})
.catch(function() {
  console.log('problem'); process.exit(1);
});

[SohumB]

That does indeed work.

But I'm mostly trying to pull together a test case for a different bug, where I can see knex use __cid: 1 and __cid: 24, even though I've set the pool to {max: 1, min: 1}, and then hang. I figured the easiest way to force knex to use multiple connections would be to use multiple transactions.

I can try and dig further into this to make a different test case, if you like.

[tgriesser]

All transaction connections should be pulled from the pool, because if they're not you lose the convenience of having the pool handle the max number of connections to a given client, if you're running a whole bunch of transactions at once you could create and number of connections and exceed the max allowed connections for the database.

I'd think that you wouldn't want to create a transaction within a transaction, because that doesn't make much sense and would defeats the point of using a transaction in the first place (you'd be operation operating on a connection using a potentially inconsistent view of the state of the database).

Are you running into this using WebSQL? Looking at the code, that's the only place I can see that it might be creating incorrect cid's because there is currently no pool in WebSQL

[SohumB]

After more investigation, that different test case appears to have been an issue on my end, so cheers!

And yes, you're right, nested transactions are Incorrect. The correct way to handle that, in postgres at least, is with SAVEPOINT, which knex doesn't support, so that's okay.

[tgriesser]

Yeah, I hadn't gotten around to it yet but I'd like to add support

[morungos]

I'll venture to put a differing point of view on nested transactions. They are useful, and I've just been stumbling around this issue, and would have liked this in knex. There are several reasons.

First, in a large system, I shouldn't need to impose constraints on someone else's components. If they provide a function which uses a transaction, i should be able to call it, even as part of my transaction. Otherwise I lose encapsulation.

Second, for testing, it is fairly common to run tests in transactions, rolling them back to ensure consistent state around each test, while each test may use transactions internally. I shouldn't need to drop and reload the whole database for each test, despite the fact that most JS frameworks expect that. Java-based frameworks often work this way, and that too depends on nesting transactions, at least in some form.

I'll improvise something with savepoints (which I can do, because I only need this in testing, and so I can make assumptions about the client) but conceptually, there really is nothing awful about nested transactions.

[tgriesser]

Just reopened to remind me to take a look at this.

[juanimarchese]

I was writing some tests, and specially when you combine before and after with beforeEach and afterEach, SAVEPOINTS are very usefull!

Hope someone add this features in the future! Bookshelf should add this as well!

[chrisbroome]

@juanimarchese They should already be supported via transaction.transaction():

knex.transaction(transaction1 => {
  return transaction1.transaction(transaction2 => {
    // do something ...
  });
});

[shinzui]

Is this still an issue? Does knex support savepoints now?

[jurko-gospodnetic]

Yup, knex supports savepoints, as mentioned in #318 (comment).

[shinzui]

@jurko-gospodnetic I am curious why the issue is still open then. I am new to knex and bookshelf, and while I really like the libraries so far, I am having a hard time navigating the issues. It's hard to tell from reading the conversations which items are actual issues.

[jurko-gospodnetic]

Let's close it and see if anyone yells :-)

P.S. I confirmed it on the internal maintainer chat first :-D

[kaplona]

Its exciting to see that SAVEPOINTS are supported by knex, but it's really hidden. There is nothing in the documentation about it. Would be nice to have #318 (comment) example in the docs, so people don't need to go fishing for answers in github issues.

Note that Amazon Redshift does not support savepoints in transactions

This is literally the only line that is there about SAVEPOINTS in the docs.