Are we ensuring mutually exclusive access to each CFD in the database?

[luckysori]

I think we are already implementing mutual exclusion when writing to the database because we only have one call to append_event in production:

https://github.com/itchysats/itchysats/blob/1305340f1fbb8567ed72f594bbf03013cbae93da/daemon/src/process_manager.rs#L74

Since the process_manager::Actor can only handle one message at a time, we cannot be calling this line in parallel. Interestingly, we cannot do it for different CFDs either! I have no idea if that is a problem, but different CFDs are orthogonal, so we should be able to modify them at the same time.

However, we are not preventing reading a CFD from the database whilst we're writing to that very same CFD, as far as I can tell. Am I right? If so, can it be a problem?

Originally posted by @luckysori in #2651 (comment)

[bonomat]

SQLite is transactional, so multiple write operations should work in isolation. Afaik we use transactions already all over the place and i think we only have one open connection to the db. Soo this should be fine. What might be missing is to handle roll back errors and retry them... but I'm not sure we have ever encountered them.

Reading should be possible concurrently: this depends on the underlying file system.

[holzeis]

Yes SQLite creates locks implicitly when you perform inserts, updates, deletes. https://docs.oracle.com/cd/E17076_05/html/bdb-sql/lockhandling.html.

My point is the following. Assume the following transaction below

Transaction

1. Read CFD
2. ... change cfd ...
3. Update CFD
4. commit()

if you have two threads executing that transaction (or other but similar transactions), you only get a lock at step 3. Hence you should already ensure mutual exclusion before step 1. otherwise you will run in concurrency issues e.g. dirty writes.

[bonomat]

Thanks for sharing the link. Very useful read :)

@da-kami : I believe we have seen DB locked (or similar) errors before right?

[luckysori]

I don't think we have concurrent writes by virtue of only being able to modify the CFD in one place (as mentioned above). Is it still a problem if we can write and read at the same time?

[da-kami]

Yes SQLite creates locks implicitly when you perform inserts, updates, deletes. https://docs.oracle.com/cd/E17076_05/html/bdb-sql/lockhandling.html.

My point is the following. Assume the following transaction below

Transaction

1. Read CFD

2. ... change cfd ...

3. Update CFD

4. commit()

if you have two threads executing that transaction (or other but similar transactions), you only get a lock at step 3. Hence you should already ensure mutual exclusion before step 1. otherwise you will run in concurrency issues e.g. dirty writes.

Thanks for the link, that was indeed interesting.

I dug a bit into the sqlx implementation and noticed that transactions are started with BEGIN and not BEGIN ITERMEDIATE, see:

https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/sqlite/transaction.rs#L13-L15

https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/sqlite/connection/worker.rs#L157-L167

https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/transaction.rs#L218-L224

I am not sure how the safepoint would behave in case there is already a tx depth of >0.
It might be worth raising the question if BEGIN IMMEDIATE is not more appropriate here; however, from the observed behavior of our system we do not have read problems.

We do see DB locked, but as far as I observed this is always related to concurrent writing, which can e.g. happen when we move cfds to the failed_cfds and closed_cfds table, see these logs

I don't think we have concurrent writes by virtue of only being able to modify the CFD in one place (as mentioned above). Is it still a problem if we can write and read at the same time?

Yes, this is currently ensured by updating open cfd data by the fact that we only have one instance of the process_manager::Actor. This actor is supervised, so the address will be ensured to be the same upon restart.
Since all writes to EVENTS table (and the rollover related tables) are handled by this actor, and we await append_event being processed (and don't push it into a task) there are not concurrent writes to that table.
Note that all writes are still dependent on events happening in the system, i.e. all data updates / inserts for open cfds are handled by the one process_manager::Actor.

What might be missing is to handle roll back errors and retry them... but I'm not sure we have ever encountered them.

For moving cfds into closed_cfds and failed_cfds this is handled in the sense that we will just retry this task, and if the transaction was rolled back we will try again once triggering the task to move closed/failed cfds.

Note that we might also have a problem of concurrent writes when creating a Cfd initially. This would result in a failed Cfd on one side.

It would be interesting to discuss if database access can be improved by re-designing how/when we access the database and potentially a better retry mechanism within the db layer.

[da-kami]

As for the original question...

However, we are not preventing reading a CFD from the database whilst we're writing to that very same CFD, as far as I can tell. Am I right? If so, can it be a problem?

I don't think it should, unless we changed how the application triggers data updates when persisting data.
When we introduced the process manager we followed the pattern of first ensuring that the event was saved, and then reacting in the sense of post-processing (e.g. publishing transactions) and sending updates to the projection.

Given this order this should be fine, because only when the data is saved we tell the application to update it's state.

Note that we do have read requests that are independent of process_manager::Actor. If we require information about a Cfd we do load it in some places, and it can happen that the state that we load is not up to date at that point because the process manager is still saving data. Usually this should not cause any problems because if we e.g. trigger a protocol, but at the end of the protocol it turns out the operation was actually not possible anymore we just fail it. In an asynchronous application this is quite normal.
We could still discuss if this can be implemented more explicitly, because at the moment these constraints are not immediately obvious and it can be hard to follow the application flow and decision making.

Note that information not being available in the database yet can become a problem if the event and the event-data are not available at the same time! This is what happened for our No Dlc available issue:

We know that we trigger loading an open cfd from the database outside the process_manager::Actor triggering updates. Given that the db-cache in load_open_cfds evaluates if there are new events in the database, but the rollover data is not part of the event, we have to ensure that we save the event and the data at the same time, otherwise a load may load an event without data.