Use transactions for asset table upserts #142
Conversation
danenbm
requested review from
NicolasPennie,
linuskendall,
febo,
blockiosaurus and
Juanito87
| .await?; | ||
|
|
||
| // Close out transaction and relinqish the lock. | ||
| multi_txn.commit().await?; |
There was a problem hiding this comment.
Why not include the other updates below?
There was a problem hiding this comment.
I didn't want to lock all the different tables at first. Starting with asset seemed like a good incremental improvement since it had the most upserts.
There was a problem hiding this comment.
Incorporated all these changes into #134
| upsert_asset_with_seq(&multi_txn, id_bytes.to_vec(), seq as i64).await?; | ||
|
|
||
| // Close out transaction and relinqish the lock. | ||
| multi_txn.commit().await?; |
There was a problem hiding this comment.
You need to know how to handle errors here. what if the transaction fails? Need to have recovery scenarios in that acse surely?
There was a problem hiding this comment.
So the way it works with SeaORM, if the the transaction (multi_txn in this case) goes out of scope, it is rolled back. Therefore, the way the code is structured, if there is an error on one of the executions of the multiple statement transaction, there will be an early return, multi_txn will go out of scope and the transaction will be rolled back.
The reported error will be the one that caused the early return, which is how it functions today. So I think the only difference will be that the asset table changes are tied together, and in the case of failure, all of them will be rolled back together. I don't know if this could be considered better behavior than today or not; it seems about equivalent to me, because either way some of the asset updates for an instruction were missed, and the asset state overall would be missing some updates.
| @@ -67,9 +67,12 @@ where | |||
| let tree_id = cl.id.to_bytes(); | |||
| let nonce = cl.index as i64; | |||
|
|
|||
| // Start a db transaction. | |||
| let multi_txn = txn.begin().await?; | |||
There was a problem hiding this comment.
this is too late to start the TXN. The TXN should include asset_should_be_updated and asset_should_be_updated should be using SELECT ... FOR UPDATE instead of the SELECT only. This would allow you to lock the rows you are selecting and fail transactions that cause a raise condition.
There was a problem hiding this comment.
Understood, I see how the txn could solve issue with asset_should_be_updated
There was a problem hiding this comment.
Race condition fixed in #134
| .await?; | ||
|
|
||
| // Close out transaction and relinqish the lock. | ||
| multi_txn.commit().await?; |
There was a problem hiding this comment.
Need to ensure that we know fully what happens if the TX fails and the reason for the TX failing. Is this a recoverable TX error or not?
There was a problem hiding this comment.
Does my explanation above for how SeaORM transactions are rolled back when the object goes out of scope answer the question? Is there some type of recovery we should aim to do, which is different then the current code that afaik just ignores errors and continues on?
|
These changes have been incorporated into Version 1 of the |
Notes
Testing
mintV1indexing.Test Results
mintV1from the range of 9-23 ms down to 2-4 ms.Test Data
Test case 1: Existing code from #135
Over 3 tries saw ~9-21 ms for the 5 upserts.
Here are the results for one trial (~11-12 ms for the 5 upserts):
Test case 2: Added semicolons after first 4 upserts to attempt SQL batching
Could not find any documentation from PostgreSQL indicating this would result in chaining. From the results it looks like this had no effect.
Over 3 tries saw ~11-23 ms for the 5 upserts.
Here are the results for one trial (~16-23 ms for the 5 upserts):
Test case 3: Code from this PR using transactions.
Over 3 tries saw ~2-4 ms for the 5 upserts.
Here are the results for one trial (~2-3 ms for the 5 upserts):