Questions inline, leaving as a comment so that the diff stays on other reviewers' queues

Do you mean only the pragma setting user version or the entire scope?

I meant everything that happens in the if (current_user_version == 0) { block.

It throws an error that is being handed further.

Can you link where it's handled?

If we set journal mode here do we also need to set it when running migrations? Isn't calling enable_write_ahead_logging_mode redundant now? I just want to make sure we do not perform redundant operations.

Moved setting journal_mode only to fresh install logic.

It throws an error that is being handed further.

Can you link where it's handled?

One note: I was meaning handed, like pass on/forwarded like any other error thrown in SQLiteQueryExecutor - not handled.

In general sqlite_orm performs sync_schema with simple CRUD operations which may throw std::system_error.

But to answer, the code we talking about happens in the migrate() function which is called in the constructor of SQLiteQueryExecutor or in the method responsible for deleting the database.
So it depends on usage of DatabaseManager::getQueryExecutor() - I saw places surrounded by try {} catch {} and without, so probably in some cases app is killed. I am not familiar enough to give a very descriptive answer and want to respond quickly so if I am wrong, @marcin could you correct me?

I meant everything that happens in the i`f (current_user_version == 0) {` block.

Honestly, I don't feel it's a good idea. ORM will create tables using standard SQLite operations and according to SQLite docs "Any command that accesses the database (basically, any SQL command, except a few PRAGMA statements) will automatically start a transaction if one is not already in effect.". By doing that we will change that default behavior and make everything that sqlite_orm doing into one huge transaction, which will need to be handled separately but thong have a strong opinion what is better.

I think we need to understand what's going on here better. It's clear from the discussion in ENG-1910 that our understanding is incomplete, and I'm concerned that we're simply not going deep enough here.

I have two questions below. We probably don't need to block landing this diff on them, especially because the original issue is blocking releases. @kamil, maybe you can create a task for each question, and follow-up in the task with the answer?

I saw places surrounded by try {} catch {} and without, so probably in some cases app is killed. I am not familiar enough to give a very descriptive answer and want to respond quickly so if I am wrong

Can you do a thorough analysis of all callsites please? I think you should be able to do this on your own, and @marcin can correct you if you're wrong :)

In D5292#156364, @kamil wrote:

Honestly, I don't feel it's a good idea. ORM will create tables using standard SQLite operations and according to SQLite docs "Any command that accesses the database (basically, any SQL command, except a few PRAGMA statements) will automatically start a transaction if one is not already in effect.". By doing that we will change that default behavior and make everything that sqlite_orm doing into one huge transaction, which will need to be handled separately but thong have a strong opinion what is better.

Can you do a thorough analysis of how sqlite_orm handles sync_schema and whether it creates a transaction for everything within that? The fact that SQLite starts a transaction per query if one isn't already in effect doesn't seem relevant here.

In D5292#156364, @kamil wrote:

Honestly, I don't feel it's a good idea. ORM will create tables using standard SQLite operations and according to SQLite docs "Any command that accesses the database (basically, any SQL command, except a few PRAGMA statements) will automatically start a transaction if one is not already in effect.". By doing that we will change that default behavior and make everything that sqlite_orm doing into one huge transaction, which will need to be handled separately but thong have a strong opinion what is better.

Can you do a thorough analysis of how sqlite_orm handles sync_schema and whether it creates a transaction for everything within that? The fact that SQLite starts a transaction per query if one isn't already in effect doesn't seem relevant here.

@kamil and I discussed this in our 1:1 today.

1. We concluded that sqlite_orm does not wrap sync_schema in a transaction.
2. We thought a bit about whether this could cause issues, but our conclusion was that an error in sync_schema would throw a system_error, which would prevent user_version from being incremented. Then we would expect sync_schema to run again on the next start, which would hopefully have the same effect as wrapping the whole thing in a transaction. (sync_schema is meant to be idempotent.)
3. We also decided we should call sync_schema(true) instead of sync_schema(). By setting the preserve parameter to true, we make the call a little safer. (Even though we don't expect the call to ever happen with an existing database.)
4. We're likely going to deprecate sqlite_orm soon anyways, so we didn't want to go too deep on this.

Only follow-up on this one is to update sync_schema() to sync_schema(true), which can be done in a separate diff. Meanwhile, the earlier discussion about errors and safety can be continued in ENG-1974.