Rethink client transaction API

[1st1]

@tailhook suggested in Slack:

I'd like to have queries executed on the transaction object itself rather than on connection:

with conn.transaction() as t:
    t.execute("xxx")

Or

conn.transaction((t) => {
    t.execute("xxx")
})

The reasons are:

1. Passing transaction to helper functions explicitly (rather than connection which might have active transaction or might have not) looks better

2. This prevents things accidentally get into transaction, if connection is any kind of global and any function can call it.

3. We're likely do that in Rust anyway, as we need to borrow a connection for the transaction.

4. This allows pool.transaction() directly rather than two layers of with.

5. Basically, there is no need to use connection in 99% apps, after this change, it's either pool.query() or transaction.query(). So we get rid of the pattern of acquiring a connection for a long time and holding it (for a duration of HTTP request for example).
   (and if we're going to fix this we should also do "repeating a transaction" by default too)

[1st1]

I like the idea.

Few comments:

This prevents things accidentally get into transaction, if connection is any kind of global and any function can call it.

Not sure how it would prevent "accidental" transactions and what they are.

This allows pool.transaction() directly rather than two layers of with.

That's a nice improvement indeed.

Basically, there is no need to use connection in 99% apps, after this change, it's either pool.query() or transaction.query(). So we get rid of the pattern of acquiring a connection for a long time and holding it (for a duration of HTTP request for example).

Yes, I can see that, provided we can have clear tutorials suggesting the pattern.

---

There are also some modes of operation that are valid only within a transaction, like creating a cursor. If we implement this proposal, the cursor() method will only be implemented on the transaction object, gently nudging users towards using the db correctly.

One more idea: perhaps we can add a way to acquire a connection from the pool without transaction() method. And create a pool without transaction() and acruire() methods. That way you can ensure in one place that all connections are short lived.

[tailhook]

This prevents things accidentally get into transaction, if connection is any kind of global and any function can call it.

Not sure how it would prevent "accidental" transactions and what they are.

Like this:

connection = None

@cached
def site_info():
    return connection.query_row("SELECT SiteInfo {...}")

def request():
    global connection
    with pool.acquire() as conn:
        connection = conn
        with conn.transaction():
            site = site_info()
            do_something(site)

Here SELECT SiteInfo is sometimes a part of transaction, sometimes is not, depending on how lucky the request is to be the first one. While it should be an independent singleton that fetches and populates the cache.

While this extra select looks innocent, there can be writes like INSERT .. UNLESS CONFLICT or even more complex things.

Note, I don't think this is always wrong. Just have to be explicit. So if we have an abstract base class, say Query, that is implemented for all three: Pool, Transaction, Connection. And function that is not independent of current transaction can accept do_domething(db: Query), and be used either with pool or transaction. And similar thing can be put into a global variable or context.

[1st1]

While I think this example is a bit contrived, I see what you mean. Point taken.

[1st1]

@tailhook

(and if we're going to fix this we should also do "repeating a transaction" by default too)

I'm not sure I like the idea of doing that by default. But can you elaborate a little on how you would do this?

[tailhook]

Basically this: https://www.cockroachlabs.com/docs/stable/transactions.html#client-side-intervention-example
Except, that is more pseudo code, so we also need at least:

1. Also retry on connection error (so that client reconnects gracefully in the middle of transaction, this by the way is another reason why we need transaction() method on pool rather than connection.
2. Better error reporting than what's in example

[1st1]

Basically this: https://www.cockroachlabs.com/docs/stable/transactions.html#client-side-intervention-example

Yes, but IIRC you once proposed a more high-level API than what they show in their pseudo-code, which as is won't be used by anyone as it's too verbose.

[tailhook]

Basically this: https://www.cockroachlabs.com/docs/stable/transactions.html#client-side-intervention-example

Yes, but IIRC you once proposed a more high-level API than what they show in their pseudo-code, which as is won't be used by anyone as it's too verbose.

I don't remember anything like that. For common case API should look like current one in JavaScript (but documented that it may run function more than once). For Python, yes, it changes more, because Python doesn't have inline functions. We probably can use decoratorish syntax to make it simpler.

Sometimes tweaking transaction parameters are needed, though. That may be more complex, but that shouldn't be common.

[tailhook]

Continued in #1738

[CodesInChaos]

One important note for the rust version: drop can't return an error. So committing a transaction needs to be an explicit call (which consumes the transaction), while dropping a transaction should roll it back (and swallow any errors happening during rollback).

[tailhook]

2. I'm not sure this is "everything that is notice-level", we may want an enum, similar to error handling spec (or that's too much)

More context: we probably will need another notice/warning when transaction was repeated (if we implement that as a builtin). And we should give them a way to differentiate between transaction.rollback() is skipped and transaction was repeated without matching the message. This should be useful for gathering run-time statistics.

[CodesInChaos]

I'd pass in a mix of general parameters (e.g. the connection), and event specific parameters. Probably something like:

struct NoticeContext<'t> {
    event:NoticeEvent,
}

struct TransactionDroppedEvent<'t> {
    transaction:&Transaction,
}

enum NoticeEvent<'t> {
    TransactionDropped(TransactionDroppedEvent),
    ...
}

fn notice_callback(self:Builder, impl Fn(&NoticeContext));

If the user wants a backtrace they can create it themselves. The default would probably just print to stderr. Personally I'd probably use some variant of the tracing crate, and use its spans to figure out which context the transaction was associated with.

[tailhook]

Yes something like this. The problem is that transaction object per se doesn't mean anything, i.e. there is nothing that could differentiate one transaction object from another saying which part of the code uses it or whatever.

Is there any good tracing lib that works with async? We should probably look into how to integrate that.

[CodesInChaos]

Is there any good tracing lib that works with async?
From what I read, the tracing crate (or one of its support crates) can link events to the surrounding span, even in async code. I did not try it myself though. Not sure if this relies on tokio or also works with async-std.

We should probably look into how to integrate that.

I'm not a fan of low level libraries integrating with a logging framework. I prefer it providing the necessary callbacks which the user can link up with the logging framework.

[tailhook]

I'm not a fan of low level libraries integrating with a logging framework. I prefer it providing the necessary callbacks which the user can link up with the logging framework.

Sure, most of the time yes. But it's unclear what needs to be done. Even if nothing, we might want to give example in the docs for usage of such libraries, because it doesn't look like a trivial task to identify transaction in a callback.

[tailhook]

We also should think how sub-transactions should be done:

with transaction.savepoint('xxx') as savepoint:
    savepoint.query('...')

I.e. I would temporary block respective methods in Transaction too.

[elprans]

Hm. Savepoints aren't really sub-transactions. You can't "commit" a savepoint, and savepoints may overlap and get released or rolled-back to arbitrarily. You can emulate a nested transaction with savepoints, but we probably shouldn't be using the savepoint name for this.

[1st1]

You can emulate a nested transaction with savepoints

That's precisely what we're doing with our API to make it composable. I don't like the idea of exposing "savepoint" terminology in a high-level API. We could call "nested" transactions something else, like "blocks" to make it explicit what's really going on there.