Subject: Re: Nested transactions support for code composability

From: Daniel Fortunov <[email-masked]>
Date: Sat, 15 Feb 2020 17:55:49 +0100

Based on the motivations outlined in the below message to this list (3
years ago), I have implemented a Transaction context manager that provides
what I think is the most intuitive way to deal with transactions.
(Functionally similar to xact / Django atomic, but available directly on
psycopg2 connections)

We have been using this in production for over a year in a fairly large
codebase that does a lot of database interactions directly using psycopg2
(without any other libraries layered on top).

I'd always though that this would be a good candidate for inclusion in
psycopg2, and now that it is stable, I'd like to solicit feedback.

This Transaction context manager replaces the DB-API model of handling
transactions (which I have found to be not very intuitive, with frequent
unexpected pitfalls, and does not lend itself to code composability) with a
context manager that provides:
1. Seamless support for nested transactions (implemented using Postgres
savepoints).
2. Inner transactions are truly independent (meaning that a unit of code
can decide whether to commit or rollback its own changes, without
influencing any outer transaction(s), or even needing to be aware if outer
transactions exist or not)
3. Transaction contexts function correctly inside "classic" transaction
management, meaning that you can implement this incrementally, starting at
the innermost libraries and working outward.

For a motivating example, please see my original email to this list (below).

For details on the library, see the README at
https://github.com/asqui/psycopg-nestedtransactions

Do people think this is worthy of inclusion in psycopg2.extras (or
elsewhere in psycopg2?)

Thanks in advance,
Daniel Fortunov

On Mon, 16 Jan 2017 at 23:26, Daniel Fortunov <
[email-masked]> wrote:

> I'd like to implement support for nested transactions in psycopg2 using a
> context manager that internally uses postgres savepoints to implement the
> ability to nest transactions within each other, with sensible commit and
> rollback semantics.
>
> The end goal is to allow code composability through support for nested
> transactions.
>
> Before I go ahead and implement this I wanted to solicit feedback on the
> suggestion, because I find it hard to imagine I'm the first person to dream
> up this idea, so I imagine I'm missing something. (Perhaps it's already
> implemented somewhere that I've not found, or maybe it's not implemented
> because it's a terrible idea, and if so I'd like to understand why!)
>
> The rest of this email describes what I'm trying to achieve, using some
> motivating examples.
>
> Please share your thoughts and suggestions.
>
> Thanks in advance,
> Daniel Fortunov
>
> ------------------
>
> Suppose we have a database storing data about widgets, and we'd like to
> implement a function that does some updates on a few related widget tables.
> We'd like to do this atomically, so we wrap it in a transaction.
>
> Let's assume that `Transaction` is a psycopg transaction context manager
> with support for nested transactions. It begins a transaction on __enter__,
> commits it on __exit__, and rolls back if there is an exception. (This is a
> simplified version of what I propose to implement.)
>
> def update_widget(cxn, widget):
>     with Transaction(cxn):
>         cur = cxn.cursor()
>         cur.execute(...)  # Update table A
>         cur.execute(...)  # Update table B
>         cur.execute(...)  # Update table C
>
> So far this works fine, and is also currently achievable with the
> 'autocommit' paradigm. Nothing new here. Anyone can call this code, provide
> a connection, and get an atomic update of tables A, B, and C.
>
> Now, I'd like to write a maintenance job that updates a whole load of
> widgets. If the updates on any one widget fail, I'd still like the job to
> continue, and process as many widgets as it can. Specifically, I want to
> implement this job _without_ changing the existing maintain_widget()
> function, which is already in use.
>
> maintain_widgets(cxn):
>     with Transaction(cxn):
>         for widget in widgets:
>             try:
>                 update_widget(cxn, widget)
>             except:
>                 pass  # log exception and continue with other widgets
>
> The Transaction context manager would implement the begin/commit/rollback
> transaction semantics by beginning/releasing/rolling back to a savepoint,
> respectively. In this context, update_widget would seamlessly switch to
> using savepoints is it is not the outermost transaction.
>
> This is not possible to achieve with the 'autocommit' paradigm, because
> you need the ability to rollback the update of a single widget, and then
> continue the transaction that updates the rest. It is possible to implement
> with explicit savepoints, however that requires maintain_widget() to be
> updated, and breaks existing code, because it will no longer be possible to
> call maintain_widget outside of a transaction!
>
> Now I want to take it a level further, and write a database integration
> test for my widget maintainer script, which executes the test scenario
> within a transaction, guaranteeing that the database is left untouched
> after my test, regardless of whether the test succeeds, fails, or aborts
> unexpectedly (e.g. maybe the test is running in an interactive debugger and
> the I abort the debugging session! :-)
>
> def test_maintain_widget_successful_run(self):
>     with Transaction(cxn) as txn:
>         # Set up starting database state
>         # ....
>         maintaint_widgets(cxn)  # Call code under test
>         # Check database state against expectations
>         # ...
>         txn.rollback()  # Rollback all changes
>
> Note that for this final ROLLBACK to work, none of the called code is
> allowed to execute COMMIT at any point. Which is why this breaks down
> completely with the autocommit = False; cxn.commit() paradigm, because we'd
> be calling commit() explicitly all over the place!
>
> With the current paradigm you're forced to manage transactions at the
> uppermost level, which means callers to maintain_widgets() are obliged to
> call it within a pre-existing transaction in order to get correct behaviour
> (namely, atomic update of all three tables in the face of errors). And even
> then, this precludes having a test one level above, which executes that
> "uppermost level" code within a transaction and then rolls everything back!
>
> So, what am I missing? Am I the first to have this crazy idea? Is this
> already implemented? How do others solve the problem of code composability
> and transactions? Is this a terrible idea? (if so, why?)
>
> Feedback and comments welcome.
> Thanks for reading this far!
>

---

From: Reuben Rissler <[email-masked]>
Date: Sat, 15 Feb 2020 13:24:19 -0500

> Do people think this is worthy of inclusion in psycopg2.extras (or 
This is interesting, to say the least. I use psycopg2 a lot, but I never 
got around to researching psycopg2 for this type of action. I can't help 
you out on how to get this into psycopg2 officially, and I don't even 
know if / when I'll use it, but it is nice to know I'm not the only one 
who has thought in patterns like this.

Reuben

---

From: Daniel Fortunov <[email-masked]>
Date: Sat, 15 Feb 2020 19:38:32 +0000

Is this worthy of inclusion in psycopg2.extras (or elsewhere in psycopg2?)

Thanks in advance,
Daniel Fortunov

---

From: Christophe Pettus <[email-masked]>
Date: Sat, 15 Feb 2020 12:55:08 -0800

> On Feb 15, 2020, at 10:24, Reuben Rissler <[email-masked]> wrote:
> This is interesting, to say the least. I use psycopg2 a lot, but I never got around to researching psycopg2 for this type of action. I can't help you out on how to get this into psycopg2 officially, and I don't even know if / when I'll use it, but it is nice to know I'm not the only one who has thought in patterns like this.

This has been implemented by the Django atomic() context manager for quite some time, in fact.
--
-- Christophe Pettus
   [email-masked]

---

From: Daniele Varrazzo <[email-masked]>
Date: Sun, 16 Feb 2020 11:52:29 +0000

> (Functionally similar to xact / Django atomic, but available directly on

...

> Is this worthy of inclusion in psycopg2.extras (or elsewhere in psycopg2?)

Hello Daniel, thank you for the idea, and I would like to provide such
functionality in psycopg.

My doubts are around the interface: not so much relatively to your
code/design, but rather relatively to the DBAPI requirements.

In short, there had been an emerging pattern of drivers using the
connection context manager to manage transactions (see e.g. psycopg
behaviour: https://www.psycopg.org/docs/usage.html#with-statement). This is
sort of a de-facto behaviour, but it never got written down in the DBAPI
specs

https://mail.python.org/pipermail/db-sig/2012-November/thread.html

In hindsight, I think it was the wrong decision:

- people expects Python objects to be closed on exit. Closing the tx but
not the connection is a surprising behaviour
- providing different connection factories, for instance a 'with
pool.getconn():... ' which would return the connection to the pool on exit -
a rather elegant design -  would require an extra level of with, see the
thoughts in https://github.com/psycopg/psycopg2/pull/17

So my thoughts are mostly: what is the best interface psycopg can present
to offer:

- transactions when requested (on autocommit requests too, see
https://github.com/psycopg/psycopg2/issues/941)
- nested transactions
- a non surprising behaviour on __exit__

In my ideal world, the behaviour should be something like:

    with connect('dsn') as conn:  # or with pool.getconn() etc.
        with conn.transaction() as tx:
            with conn.cursor() as curs:
                stuff()
                # conn.transaction() might be called again to create
savepoint tons
                # tx might expose commit()/rollback() for explicit
management

            # dispose if the cursor
        # commit on success, rollback on error
    # close the connection, return it to the pool, etc

Would it be possible to introduce this behaviour in psycopg2 in a way that
is not awful, which wouldn't break programs written for the 2.5-2.8
behaviour? I don't see an obvious way to do it.

If not, and we had to introduce a non backwards compatible change, does the
design above seem optimal (complete and easy to use)?

For a full disclosure: in the next months my work situation should change
and I might be able to start working on a new major psycopg version, so
psycopg3 might actually become a real thing, if there is interest for it.

-- Daniele

---

From: Daniel Fortunov <[email-masked]>
Date: Mon, 17 Feb 2020 08:26:40 +0000

> Would it be possible to introduce this behaviour in psycopg2 in a way
that is not awful

I think there are two separate pieces here that maybe can be treated
independently:

1. Introduction of conn.transaction() for explicit transaction management,
including support for nested transactions.
This addresses the first two out of the three desired interface features
you've listed, and is a non-breaking change.

2. The desire to replace `with conn` semantics with something more
intuitive (i.e. closing the connection)
This is certainly what one would intuitively expect (including me) -- I
wasn't aware that `with conn` semantics for transaction management were
adopted as a defacto DBAPI behaviour. I think this is a poor design choice
as it is not very intuitive.
Making __exit__ close the connection, however, is a breaking change and
would likely have to wait until psycopg3. The upgrade path for existing
code would also be horribly painful...
Perhaps a hybrid approach would be best: Have __exit__ commit an ongoing
transaction (if there is one) provided there was no exception, and then
close the connection. This would at least give you a clear error on the
next attempt to use the connection, and prevent data loss from closing a
connection with an uncommitted transaction.

In terms of dependencies, I think implementing 2 requires 1 (you need to
have a better transaction management construct to migrate existing `with
conn` code to), but 1 does not necessarily require 2. i.e. You can
introduce `conn.transaction()` usage to existing code, whilst leaving
management of connection lifetime the same as your code does it today.
(This is effectively what we have done in our codebase, by introducing
`with Transaction(conn)` to existing code, regardless of the fact that
`with conn` also does some transaction management.)

The only downside of doing 1 without 2 is that this leaves the API design
less clean, because now you have two ways to do transaction management, but
I think this is preferable to coupling 1 and 2 and only being able to
introduce both of them in psycopg3.

The only other avenue I can think of is to buy-in to the "`with conn` for
transaction management" pattern even more (for psycopg2 at least), and
introduce support for reentrancy such that you can nest `with conn` blocks
for savepoint-based nested transactions. This is the only approach I can
think of that maintains backward compatibility with psycopg2, but it has
all the same drawbacks of not being intuitive. (Sadly, actually, this is
only "backward compatible" in terms of "design spirit" not actually in
terms of implementation -- currently nested `with conn` blocks will result
in the deepest block committing (or rolling back) the entire transaction,
and subsequent statement execution in the outer block(s) beginning a new
transaction. We found a lot of weird edge cases like this when migrating
our codebase to use `with Transaction()` and having autocommit enabled by
default -- such cases that are probably conceptually "broken" but
nonetheless "work" currently.)

Aside: Are you coming to PyCon US in April? It would be great to discuss
some of these topics face-to-face.

Regards,
Daniel

On Sun, 16 Feb 2020 at 11:52, Daniele Varrazzo <[email-masked]>
wrote:

> behaviour: https://www.psycopg.org/docs/usage.html#with-statement). This
> is sort of a de-facto behaviour, but it never got written down in the DBAPI
> specs
>
> If not, and we had to introduce a non backwards compatible change, does
> the design above seem optimal (complete and easy to use)?
>