Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
1262: feat(dal,sdf,si-data): introduce "cloneable" Postgres transactions r=fnichol a=fnichol This change alters some of the internal details of our `pg` module in `si-data` which is an adapter layer on top of the `tokio-postgres` and `deadpool-postgres` crates. The change is largely in the `InstrumentedTransaction` type which now holds an `Arc<Mutex<Transaction<'a>>` (formerly this would have held a `Transaction<'a>`) allowing the type to derive `Clone`. Now we have a way of "cloning" an in-flight database transaction and pass ownership to other functions, Tokio tasks, etc. The key here is that all copies are sharing the same mutex-guarded `Transaction<'a>` type and so the consumers of these copies are coordinating within the singular transaction. As the synchronized access is all internal to `InstrumentedTransaction`, the external API did not change. *However*, there are 2 reasons leading to signature changes which consequently resulted in a large number of small changes across the dal, sdf, and si-data crates: 1. The signature of `DalContext` needs an additional explicit (but often anonymous) lifetime. Sorry! Initially when developing the `DalContext` type, the borrowed `Transactions` type used the *same* lifetime for the borrow and for the internal lifetime of the type. That is, prior to this change the field signature was `txns: &'t Transactions<'t>` which was fine because the internals of `Transactions` need to live at least as long as the borrow of `Transactions` itself. With this new change we occasionally need to describe the lifetime inside `Transactions` separately from its borrowed lifetime. That is, the field signature is now `txns: &'t Transactions<'a>` where, if necessary, the lifetimes `'a: 't` apply. This means that `'a` need to live *at least* as long as `'t` but could live longer. It is this tweak that allows our existing `txns.commit().await` to work as the `txns` is borrowed into our `DalContext`. Cool? Maybe not, but that's why! Speaking of `commit`... 2. The caller of a `pg_txn.commit().await` needs to ensure that there are no other copies of `pg_txn` that are still in scope. The prior implementation makes this possible as there was no way to clone transactions and importantly the underlying `tokio-postgres` crate's `Transaction.commit` signature consumes `self`. Our upgraded `InstrumentedTransaction` now wraps this transaction with `Arc<Mutex<...>>`, meaning we will need to also consume these data types in `commit` (note that this applies equally to the `rollback` method). To accomplish this in `commit` we are asserting that there are no more `Arc` copies and we only have 1 remaining (there is a way in Rust you can ask an `Arc` for it's "strong count" of references) and if this holds true we can safely unwrap the `Arc` and then consume the inner `Mutex` resulting in a fully consumable `Transaction` type. While this change sounds potentially troubling, in practice so far no code needed to be modified for the correct behavior of the codebase and test suites. It's my assertion that any error messages coming back on `commit` and/or `rollback` that are related to these assertion checks/unwraps are either a concurrent race calling `commit` just before other `Arc` copies are fully dropped, or it's a straight-up programmer error. See the documentation for `InstrumentedTransaction.commit` and `rollback` for more details. Unfortunately, these 2 new error conditions (one each for `commit` and `rollback`) meant introducing a proper error type which wrapped over `tokio_postgres::Error` and updating other parts of the codebase that we referring directly to the `tokio_postgres::Error` type. A nice side effect was finding that once this refactoring was done, there was no longer a reason for `lib/dal` to take a direct dependency on `tokio-postgres` which felt good and right. <img src="https://media2.giphy.com/media/ToMjGpmGUC7axrjBAmQ/giphy.gif"/> Signed-off-by: Fletcher Nichol <fletcher@systeminit.com> Co-authored-by: Fletcher Nichol <fletcher@systeminit.com>
![https://media2.giphy.com/media/ToMjGpmGUC7axrjBAmQ/giphy.gif"/](https://media2.giphy.com/media/ToMjGpmGUC7axrjBAmQ/giphy.gif"/){kind=link}
- Loading branch information
