In D5377#159644, @atul wrote:

I think this needs to be cleaned up some more before it's ready for review

Looks like while making diff I chose the wrong line to commit... I was sure it wasn't there, what is more, test case will fail with this. Anyway sorry. for taking your time and confusing you, now should work.

If there is still something confusing and the summary is not decent let me know.

1. We want to ROLLBACK when a migration fails.

I think we could just do something like this: https://phab.comm.dev/D5433?

In D5377#160227, @atul wrote:

1. We want to ROLLBACK when a migration fails.

I think we could just do something like this: https://phab.comm.dev/D5433?

That only partially fixes the problem, the problem in the previous version of code that on error we do not rollback transaction.

The whole point of this diff is to

1. Get the version
2. Perform migration
3. Update the version

in one transaction, the solution you suggested wraps only point 2 in the transaction.

More details here: https://linear.app/comm/issue/ENG-1910#comment-8e8a1fe9

The code duplication is caused only by the fact that we have to additionally support migrations that are not capable of being performed inside transactions (like setting journal_mode), but if you have any better idea on how to handle this I will appreciate any suggestion.

Answering only here because I think it covers also inline comments.

Does the following stack of two commits achieve both goals D5440 D5441?

In D5377#160412, @atul wrote:

Does the following stack of two commits achieve both goals D5440 D5441?

I think so!

The only problem is that when there is a migration that shouldn't be in a transaction checking and updating version are not in one transaction - in my solution they are.
The whole problem described in the issue I linked in the summary is probably some other thread decreased the database version and with @tomek we thought that this should reduce the risk of this scenario (I know we have different mechanisms right now like GlobalDBSingleton but we wanted to make this code as safe as possible without looking at other code dependencies).

If you @atul and @tomek think it's safe in the way Atul proposed I am happy to implement this one - much shorter and easier to understand.

Thanks for suggesting this test @marcin, I am a little bit skeptical because in the concurrent environment we will never be 100% sure it works, only if we will have luck we will find something which might not work but here is what I did:

1. Temporarily change sqlite compilation settings for both iOS and Android so that multithreaded access to SQLite is enabled.

I narrowed this only to iOS where we have multithreaded access because we testing shared code (but I can add implementation for Andriod if you think it's needed later on)

Create several threads that call DatabaseManager::getQueryExecutor() simultaneously outside of GlobalDBSingleton context. Make sure that database is not present yet at the time those constructors are executed and that migrations are actually performed by different threads (add logging which thread performed which migration)

I did this in AppDelegate on app start - it's easier and we are sure that any other getQueryExecutor call will not interfere with test, code:

[self attemptDatabaseInitialization];
NSLog(@"Clear previous db to make all migrations run");
comm::DatabaseManager::getQueryExecutor().clearSensitiveData();

NSLog(@"Starting test process...");
std::vector<comm::WorkerThread*> workers;
for(int i = 0; i < 10; i++) {
 workers.push_back(new comm::WorkerThread("worker-" + std::to_string(i)));
}
NSLog(@"Threads created");
for(auto worker: workers) {
 worker->scheduleTask([=]() {
  comm::DatabaseManager::getQueryExecutor();
 });
}
NSLog(@"Jobs started...");

If you care about logs Logger does the job for us,
2022-10-25 15:41:17.207031+0200 Comm[21320:165165] COMM: db version: 0
the value after the colon (here 165165) is the thread indicator.
I will not attach entire logs because it's a lot of data but I have those saved if anyone will be interested

1. Test the latest master
The problem is that each thread started the migration process at one time, first one started writing to the database and all others failed with the error COMM: Error creating 'drafts' table: database is locked, that being said only the first thread which did not fail, finished the migration process.

2. Test the latest master (with delay)
Assuming that all threads will start at the same time is not the best, so I am adding a delay to migrate() function

std::chrono::milliseconds timespan(1000); 
std::this_thread::sleep_for(timespan);

Now it depends on the run (I tested this multiple times) but usually, most threads fail as in point 1, however, there are some (~2, 3) threads that continue executing because they found empty slots and do not fail, part of logs:

2022-10-25 16:07:50.245311+0200 Comm[22373:184571] COMM: migration 16 succeeded.
2022-10-25 16:07:50.483249+0200 Comm[22373:184630] COMM: migration 17 succeeded.
2022-10-25 16:07:51.252446+0200 Comm[22373:184571] COMM: migration 17 succeeded.
2022-10-25 16:07:51.496106+0200 Comm[22373:184630] COMM: migration 18 succeeded.
2022-10-25 16:07:52.263907+0200 Comm[22373:184571] COMM: migration 18 succeeded.
2022-10-25 16:07:52.506195+0200 Comm[22373:184630] COMM: migration 19 succeeded.
2022-10-25 16:07:53.272700+0200 Comm[22373:184571] COMM: migration 19 succeeded.
2022-10-25 16:07:53.518424+0200 Comm[22373:184630] COMM: migration 20 succeeded.
2022-10-25 16:07:54.285700+0200 Comm[22373:184571] COMM: migration 20 succeeded.

two different threads remembered the initial database version and keep working (it won't fail because we made migrations idempotent previously) - but that's the case we want to avoid.

3. Test this diff (without delay)
Same as in point 1

4. Test this diff (with delay)
Each migration is performed only once and it's done by different threads.

In each test case database schema was created properly.

I did some changes to avoid calling applyMigration in two (mutually exclusive) conditions like @atul and @marcin prefers.

I implemented @tomek's suggestion from https://phab.comm.dev/D5377#162110.

After consulting this with @tomek we decided to divide this into two separate functions - merging this flow into one is possible but the code is hard to understand, so this should improve readability.