Savepoints with same name about rusqlite HOT 13 CLOSED

I don't believe what that test is doing is valid. Specifically, calling tx.savepoint() twice in the same scope looks like it's creating "sibling" transactions, but transactions are always nested. Since they're nested, I would expect that code to return 1; the insertion of 4 would be rolled back during the rollback of sp1.

I think the bug here is that savepoint should take &mut self, which would prevent that code from compiling and make it more obvious that the savepoints are nested.

from rusqlite.

So, your example shouldn't have compiled, but you're right that savepoints need distinct names. #151 addresses both of those.

from rusqlite.

You should have a look at the SQLite tests to see that savepoints are not necessarily nested:

       SAVEPOINT one;
          INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50;
         ROLLBACK TO one;
          INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50;
          SAVEPOINT two;
            DELETE FROM t1 WHERE (random()%10)==0;
            SAVEPOINT three;
              DELETE FROM t1 WHERE (random()%10)==0;
              SAVEPOINT four;
                DELETE FROM t1 WHERE (random()%10)==0;
          RELEASE two;

          SAVEPOINT three;
            UPDATE t1 SET x = substr(x||x, 12, 100000) WHERE (rowid%12)==0;
            SAVEPOINT four;
              UPDATE t1 SET x = substr(x||x, 14, 100000) WHERE (rowid%14)==0;
           ROLLBACK TO three;
            UPDATE t1 SET x = substr(x||x, 13, 100000) WHERE (rowid%13)==0;
          RELEASE three;

        DELETE FROM t1 WHERE rowid > (
          SELECT rowid FROM t1 ORDER BY rowid ASC LIMIT 1 OFFSET 256
        );
        RELEASE one;

from rusqlite.

Another point: maybe we should let users specify the savepoint name.
With JDBC, you have the choice:

• setSavepoint()
• setSavepoint(String)

from rusqlite.

Aren't those all still nested? E.g., this code should be equivalent:

        let mut one = try!(tx.savepoint());
        try!(one.execute_batch("INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50"));
        try!(one.rollback());
        try!(one.execute_batch("INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50"));
        {
            let mut two = try!(one.savepoint());
            try!(two.execute_batch("DELETE FROM t1 WHERE (random()%10)==0"));

            {
                let mut three = try!(two.savepoint());
                try!(three.execute_batch("DELETE FROM t1 WHERE (random()%10)==0"));
                {
                    let mut four = try!(three.savepoint());
                    try!(four.execute_batch("DELETE FROM t1 WHERE (random()%10)==0"));
                    four.set_commit();
                }
                three.set_commit();
            }

            try!(two.commit());
        }

        {
            let mut three = try!(one.savepoint());
            try!(three.execute_batch("UPDATE t1 SET x = substr(x||x, 12, 100000) WHERE (rowid%12)==0;"));
            {
                let mut four = try!(three.savepoint());
                try!(four.execute_batch("UPDATE t1 SET x = substr(x||x, 12, 100000) WHERE (rowid%12)==0;"));
                four.set_commit();
            }
            try!(three.rollback());
            try!(three.execute_batch("UPDATE t1 SET x = substr(x||x, 12, 100000) WHERE (rowid%12)==0;"));
            try!(three.commit());
        }

        try!(one.execute_batch(r#"
        DELETE FROM t1 WHERE rowid > (
          SELECT rowid FROM t1 ORDER BY rowid ASC LIMIT 1 OFFSET 256
        );
        "#));
        one.commit()

There are a couple of problems exposed by this separate from nesting:

• rollback() should take &mut self instead of self so that you can rollback and continue to use the transaction.
• We should be able to disable the "rollback or commit when transaction is dropped" behavior - in the first SQLite block, savepoints three and four are committed when one is committed, but to get the equivalent, I had to tell three and four to go ahead and commit when they were dropped.

from rusqlite.

On allowing user-specified savepoint naming - it seems like that shouldn't be necessary. Can you think of an example where it would be useful?

from rusqlite.

Ok, you are forcing the user to create two blocks when she only wants two sequential savepoints...
And a savepoint named "after_banda_sal" is more telling than "sp1":
https://docs.oracle.com/database/121/CNCPT/transact.htm#GUID-5BB15405-8A03-47DE-8A20-63E1B83E1361

from rusqlite.

I think the blocks are necessary if we want auto-commit/rollback when the transaction is dropped. Otherwise we allow the user to create transactions that look like they aren't nested, but they really are at the SQLite layer.

On savepoint names - wouldn't the variable be named after_banda_sal? If we let the user specify savepoint names, they could create two nested savepoints with the same name, which will behave oddly at best. That seems like a bad tradeoff to make to get more debuggable SQL.

from rusqlite.

Even Rust allows two variables with the same name, "which will not necessarily behave oddly at best" 😉

from rusqlite.

Hah. I was going to write about Rust doesn't really allow that and the second name just shadows the first one, making it unusable until the second goes out of scope, but that's exactly how same-name savepoints describe. Since they do nest, I suppose allowing users to specify their own savepoint name is probably fine.

Working on a PR to separate Transaction and Savepoint since their behavior is slightly different (ROLLBACK TO savepoint_foo rolls back changes but leaves you inside savepoint_foo, but ROLLBACK rolls back changes and ends the transaction). I'll add savepoint names to it.

from rusqlite.

Another point about the multiple blocks - once Rust gets non-lexical borrow scopes, you should be able to end the borrow by consuming the savepoint, allowing you to write things like:

        let mut one = try!(tx.savepoint());
        try!(one.execute_batch("INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50"));
        try!(one.rollback());
        try!(one.execute_batch("INSERT INTO t1 SELECT * FROM t1 WHERE rowid<50"));

        let mut two = try!(one.savepoint());
        try!(two.execute_batch("DELETE FROM t1 WHERE (random()%10)==0"));
        try!(two.commit()); // consumes two, releasing the borrow on one, allowing...

        try!(one.commit()); // won't compile on current rust, but should in the future

from rusqlite.

@gwenn Does #152 address the rest of your concerns?

from rusqlite.

Yes.
Thanks.

from rusqlite.