make session / core (?) docs more clear about SAVEPOINT / ROLLBACK TO SAVEPOINT / RELEASE mapping to begin/rollback/commit

[Jean-Sébastien Suzanne]

See http://docs.sqlalchemy.org/en/latest/orm/session_transaction.html#using-savepoint.

A save point can be released, it is not a commit or rollback. Or in the documentation, the begin_nested must be committed or rollbacked.

[Mike Bayer]

I don't understand this request. The term "typo" means a spelling error, errant character, or a grammatical error. I do not see any such error in the documentation linked. I don't see any text that refers to a savepoint as a commit or rollback. Please clarify, thank you!

[Jean-Sébastien Suzanne]

sorry, is not the grammatical but theoretical, begin_nested is not a save point, because i can't release it without commit or rollback.

[Mike Bayer]

begin_nested() directly emits the SAVEPOINT directive. The concept of the SAVEPOINT is abtracted into the concept of a "nested transaction". The rationale for this is so that nesting of opertations inside the scope of "rollback" or "commit" can occur without a specific transactional scope; e.g. it could be a real BEGIN/COMMIT, or just a logical nesting inside of an existing BEGIN/COMMIT, or a SAVEPOINT that acts as a rollback point.

If you could be more clear if the proposal here is simply to change the name "begin_nested()", or what, that would be helpful.

[Jean-Sébastien Suzanne]

How to release the begin_nested if i don't want commit or rollback.

If we can not release the SAVEPOINT what is the interest a simple begin is enough.

[Mike Bayer]

you'd call session.commit() while begin_nested() is active. This emits RELEASE.

[Jean-Sébastien Suzanne]

but it is commit too

[Mike Bayer]

Please try it out and turn on echo=True to understand how it works.

[Jean-Sébastien Suzanne]

Hi Mike,

I saw the release, and my mistake, it is works. But in this case the word commit perturb me. For me if i write commit i will have a writer in my db. I prefer have the word release to release the save point and commit to release the save point and commit the current transaction.

If you prefer to get this logic then close the issue, add a sentence to indicate that commit doesn't do a commit but do a release.

In my mind, add the function release to release the save point, on the session or only on the transaction will be better. but in this case the commit must release and commit.

[Mike Bayer]

OK so, the fact that begin_nested == CREATE SAVEPOINT and commit == RELEASE can be made more clear in that documentation; the docs there probably are too oriented towards the "begin/commit" abstraction placed on top of the SAVEPOINT. I was pretty sure from the beginning that the docs vagueness on this point was part of the confusion generated.

now as far as functions like session.release_savepoint(), etc., that's not a bad API but it wouldn't be consistent to just add that to Session without at least getting into Core first, where the begin/commit abstraction is used on top of SAVEPOINT in the same way; but also it starts to clutter the API with more than one way to do the same thing; users may be puzzled if they're to call commit() or release() in a given context, what's the difference, and why are there two ways if they both do the same thing (release() would have the difference that it is only available if the transactional object, e.g. SQLAlchemy Transaction or SessionTransaction, is in fact a SAVEPOINT). So I'd be -1 on actual new API being added, as I think users of SAVEPOINT probably need to be aware of the begin/commit analogy added on top of it in any case, and once that's understood they'd just use that. It's not valid in any case in SQL to RELEASE savepoints out of the nesting order in which they were created in any case (I'm pretty sure I tested this) so the "nesting" analogy SQLAlchemy places on top simplifies the general mechanics of SAVEPOINT.

[Jean-Sébastien Suzanne]

For me the if the release method is added, On the session, it is not the same functionnality:

• release: must be called only if it is a nested transaction, and release it else raise an exception
• commit: must release all the nested transactions and commit the global transaction. If they are no nested transaction, commit only the current transaction

I do not know well all the code of SQLAlchemy, but I think that the modification should be only in sqlalchemy.orm.session.Session. What do you think about this.

[Mike Bayer]

well we can't change the behavior of commit() in any such dramatic way, that ship has sailed.

[Jean-Sébastien Suzanne]

I understand, Why not juste forbid the commit if the transaction is nested, with a message as "Please release the nested transaction first". They are any change in the Core of SQLAchemy, the change are only in the session. Basically nothing change juste the name of the commande:

• release: for nested transaction
• commit: for other transaction

# in sqlalchemy/orm/session.py

def commit(self):
    if self.transaction is None:                                            
        if not self.autocommit:                                             
            self.begin()                                                    
        else:                                                               
            raise sa_exc.InvalidRequestError("No transaction is begun.")    

    if self.transaction.nested: 
        raise sa_exc.InvalidRequestError("A nested transaction is begun. Please release it before commit")

    self.transaction.commit() 

def release(self):
    if self.transaction is None or not self.transaction.nested:                                                                                                        
            raise sa_exc.InvalidRequestError("No nested transaction is begun.")    

    self.transaction.commit()

I do not test this code, And with this change lot of source refactor will be done for all soft which use SQLAlchemy. but it stop the confusion.

Maybe, the exception in the commit can be replaced by a deprecated warning.

[Mike Bayer]

this change would be completely backwards-incompatible with literally thousands of production applications, so such a change is not an option.

[Jean-Sébastien Suzanne]

I understand, replace the sa_exc.InvalidRequestError by a warning, does not break compatibility for existing application.

[Mike Bayer]

There is an existing system of begin/commit being logically nested; the SAVEPOINT sequence works within this structure so that applications can nest begin/commit blocks without worrying if the atomic unit is a transaction or a SAVEPOINT. As I said earlier, the API here is intentional. It is critical for techniques such as the "Supporting tests with rollbacks" recipe you see at http://docs.sqlalchemy.org/en/rel_1_0/orm/session_transaction.html#joining-a-session-into-an-external-transaction-such-as-for-test-suites. I am sorry if you find it to be confusing, but it is the hope that with clearer documentation this confusion may be alleviated.

[Jean-Sébastien Suzanne]

OK, thanks for all.