Conversation
tests/test_async_tx.py
Outdated
| with self.assertRaisesRegex( | ||
| edgedb.InterfaceError, "savepoint.*already exists" | ||
| ): | ||
| await tx.declare_savepoint("sp1") |
There was a problem hiding this comment.
This is contrary to server savepoint semantics, masking an earlier savepoint is allowed:
edgedb> start transaction;
OK: START TRANSACTION
edgedb[tx]> declare savepoint sp1;
OK: DECLARE SAVEPOINT
edgedb[tx]> declare savepoint sp1;
OK: DECLARE SAVEPOINT
There was a problem hiding this comment.
This releases previous savepoint with that name, right?
Not it doesn't. It creates a nested one with that name. And then you have to release twice.
So releasing the one that was created first should release inner, to have correct semantics.
There was a problem hiding this comment.
Three questions:
- Why do we skip context API:
with tx.declare_savepoint("name")entirely? - We should allow skipping name argument and generate something unique (e.g. uuid). Actually while we have variables in python (I mean
sp_name = conn.declare_savepoint()) names are useless, will only provoke unexpected conflicts. - Do we need
declare_part?x = tx.savepoint()is nice, no? (we already do that with transactions by skippingstartpart)
Context API fits in pretty nicely, even with rollback (since rollback and release aren't mutually exclusive):
with tx.savepoint() as sp:
# do something
if was_already_applied:
sp.rollback()It can be added later, but is pretty uncontroversial I think.
tests/test_async_tx.py
Outdated
| with self.assertRaisesRegex( | ||
| edgedb.InterfaceError, "savepoint.*already exists" | ||
| ): | ||
| await tx.declare_savepoint("sp1") |
There was a problem hiding this comment.
This releases previous savepoint with that name, right?
Not it doesn't. It creates a nested one with that name. And then you have to release twice.
So releasing the one that was created first should release inner, to have correct semantics.
+1
Yes, please, it should be |
|
Pushed a fix to rename
Let's add later 😏 |
|
Hi all, any updates on this? |
Sorry, not yet. But we'll put time into the Python binding very soon! |
5 participants
Please see tests for use case.
Fixes #407