Make MP mark_complete a write transaction #225
Conversation
Remove SQLiteMultipart::mark_complete(const std::string& upload_id), a unused variant of mark_complete(upload_id, duplicate) Signed-off-by: Marcel Lauhoff <[email protected]>
Move the duplicate check (MP marked completed or later) untill after the update that puts it into COMPLETE. (Ideally this would be one SQLite statement with 'returning'). This way the transaction is a write transaction and doesn't require a read to write lock upgrade and the default retry handler catches. Signed-off-by: Marcel Lauhoff <[email protected]>
| if (storage.changes() == 0) { | ||
| const auto state = storage.select( | ||
| columns(&DBMultipart::state), | ||
| where(is_equal(&DBMultipart::upload_id, upload_id)) | ||
| ); | ||
| if (state.size() == 0) { | ||
| return false; | ||
| } | ||
| *duplicate = (std::get<0>(state[0]) >= MultipartState::COMPLETE); | ||
| } else { | ||
| *duplicate = false; |
There was a problem hiding this comment.
To clarify, this only makes the operation potentially write-only, right? If the conditions are met, this will still be a read-write transaction. Or am I misunderstanding the point?
There was a problem hiding this comment.
It is a write transaction, because it starts with an update. Write transactions may also do reads. Read-then-write transactions require an upgrade to a write transaction. Starting the write transaction directly is subject to the busy timeout handler. The read-then-write in many of our cases not. They may trigger the deadlock prevention logic if multiple concurrent connections already have the read lock.
This gives us two options to get transaction retries: the busy transaction retry utility or starting with writes to use the busy timeout handler.
Since the update is the important bit here and not dependent on a previous select, making it a write transaction makes most sense I think.
Related docs:
https://www.sqlite.org/lang_transaction.html#:~:text=2.1.%20Read%20transactions%20versus%20write%20transactions
Make SQLiteMultipart::mark_complete a write transaction so that the
default retry timeout handler kicks in.
Fixes: https://github.com/aquarist-labs/s3gw/issues/710#issuecomment-1757011216
Checklist