Just checking the obvious things here:

• Was the SQLite library itself built with multithreaded support and was it enabled for the connections you made?
• Each thread/task is getting its own connection, yes? (It appears to be, but C# is not a language I'm deep into the nuances of.)
• What filesystem was the database on? Please say "local disk with NTFS" as that's a relatively well-known scenario!

1. This example uses the System.Data.SQLite library maintained by the Sqlite team here. The documentation states:
   You May create multiple threads, and those threads can create their own SQLiteConnection and subsequent objects for accessing a database. Multiple connections on multiple threads to the same database file are perfectly acceptable and will behave predictably.
2. Yes, although these Tasks are run in a thread-pool. But the using statements will make sure each SQLite connection is created and disposed of within the scope of the Task.
3. local disk with EXT4

That's fine. I was asking to make sure we weren't in any of the crazy scenarios where things are expected to go wrong.

Sounds like it needs deeper expert attention than I can provide.

Thanks for looking at this so quickly.

I was going through the code changes you made. This new comment you added seems to describe the cause of the bug.

 // BUGFIX: There are SQL statements that cause a write transaction
 //         to be started and that always require more than one step
 //         to be successfully completed, e.g. INSERT with RETURNING
 //         clause.  Therefore, if there is a write transaction in
 //         progress, keep stepping until done unless forbidden from
 //         doing so by the caller.

I can see you've only added the fix to ExecuteScalar. I was just having a play around with the 1.0.117 version and this bug also occurs using ExecuteNonQuery.
I guess the RETURNING clause inside ExecuteNonQuery is not a particularly valid use case for that API but I can see people making the mistake.
There is also has some strange behaviour where ExecuteNonQuery is returning -1 when inserting a record using the RETURNING clause instead of 1.

Also, the EF Core team thought this was an SQLite bug. How did you know it was a System.Data.SQLite bug and not an SQLite bug?

Can you please explain what caused the original bug? The comment that INSERT with RETURNING requires more than one step confuses me. I understand that stepping would be needed to collect all the output, but my understanding was that all writes were done on the first step based on the RETURNING documentation:

When a DELETE, INSERT, or UPDATE statement with a RETURNING clause is run, all of the database changes occur during the first invocation of sqlite3_step().

My reading of that is that the OP's original behavior of missing inserted rows wouldn't be fixed by adding more steps (because all the insertions are done on the first step). Am I wrong, is the documentation wrong, or is there a separate part of the fix that addresses that?

The reason I'm asking is I want to make sure I don't need a similar fix in my own code that calls the SQLite API. Also, I see the WASM OO1 API will skip extra stepping when output isn't needed so I'm wondering whether that needs attention. Thanks!

The reason I'm asking is I want to make sure I don't need a similar fix in my own code that calls the SQLite API. Also, I see the WASM OO1 API will skip extra stepping when output isn't needed so I'm wondering whether that needs attention.

FYI, investigating the equivalent in detail for the JS API is on my TODO list but a real-world relocation at the end of this week is going to delay that. The RETURNING construct never entered my mind while writing the JS code and it's quite possible that "something" needs to be done to handle that case properly. Hopefully i can say more on that by next week.

My C-language test program to reproduce this behavior is shown below. This is what the program does:

1. Open a database connection to the database "x1.db".
2. Prepare statement "INSERT INTO t1(a) VALUES(?) RETURNING rowid;"
3. Step the statement once only, then finalize the statement.
4. Close the connection.

The test database is initialized using

CREATE TABLE t1(a,b,c);

A separate process holds a read transaction open on the database file during all tests. When the database is in journal_mode=DELETE, the INSERT will fail because the database cannot be changed while another connection is holding a read transaction. (This is because any change to the database might change content out from under the connection holding the read transaction.) When the database is in journal_mode=WAL, on the other hand, the INSERT can proceed even though another read transaction is active.

When the INSERT fails because of being in DELETE mode, the sqlite3_finalize() call will always return SQLITE_BUSY. So SQLite always alerts the application that the INSERT failed. The call to sqlite_step() also always returns SQLITE_BUSY, though not necessarily on the first call to sqlite3_step(). If there is a RETURNING clause, then sqlite3_step() might return SQLITE_ROW a few times before it returns SQLITE_BUSY. It will, however, always return SQLITE_BUSY before it returns SQLITE_DONE.

So I think the bug here is in either the application, or in the System.Data.SQLite methods that the application is calling. One or the other of these codes is assuming that if the first call to sqlite3_step() on a DML statement does not return SQLITE_BUSY then the DML statement must have succeeded. This is not true. The application or library needs to either run sqlite3_step() until it completes or else check the return value from sqlite3_finalize() or sqlite3_reset() to see if the INSERT really did complete.

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include "sqlite3.h"
int main(int argc, char **argv){
  sqlite3 *db = 0;
  sqlite3_stmt *pStmt = 0;
  int v, rc;
  char *zSql;

  if( argc!=2 ){
    fprintf(stderr, "Usage: %s NUMBER\n", argv[0]);
    exit(1);
  }
  sqlite3_open("x1.db", &db);
  v = atoi(argv[1]);
  zSql = sqlite3_mprintf("INSERT INTO t1(a) VALUES(%d) RETURNING rowid;", v);
  printf("SQL: %s\n", zSql);
  sqlite3_prepare(db, zSql, -1, &pStmt, 0);
  sqlite3_free(zSql);
  rc = sqlite3_step(pStmt);
  printf("step returns %d\n", rc);
  rc = sqlite3_finalize(pStmt);
  printf("finalize returns %d\n", rc);
  sqlite3_close(db);
  return 0;
}

Thanks, that's good to understand.

The application or library needs to either run sqlite3_step() until it completes or else check the return value from sqlite3_finalize() or sqlite3_reset() to see if the INSERT really did complete.

I think the sqlite3_reset() page could mention the behavior for that special case. If sqlite3_step() returns SQLITE_ROW before it would eventually return SQLITE_BUSY, the doc says sqlite3_reset() will return SQLITE_OK:

If the most recent call to sqlite3_step(S) for the prepared statement S returned SQLITE_ROW or SQLITE_DONE, or if sqlite3_step(S) has never before been called on S, then sqlite3_reset(S) returns SQLITE_OK.

The reason I'm asking is I want to make sure I don't need a similar fix in my own code that calls the SQLite API. Also, I see the WASM OO1 API will skip extra stepping when output isn't needed so I'm wondering whether that needs attention.

FYI, investigating the equivalent in detail for the JS API is on my TODO...

(Edited to reflect changes made after this post...)

In the current trunk, both oo1.Stmt.finalize() and Stmt.reset() will now throw if the underlying sqlite3_finalize/sqlite3_reset() returns non-0. Prior to this change, finalize() ignored non-0 result codes per the long-standing wisdom that "destructors do not throw" and reset() ignored the result due to an oversight on my part.

Per Richard's explanation further down in this thread:

So I think the bug here is in either the application, or in the System.Data.SQLite methods that the application is calling. One or the other of these codes is assuming that if the first call to sqlite3_step() on a DML statement does not return SQLITE_BUSY then the DML statement must have succeeded. This is not true. The application or library needs to either run sqlite3_step() until it completes or else check the return value from sqlite3_finalize() or sqlite3_reset() to see if the INSERT really did complete.

The INSERT RETURNING case described in this thread will now trigger an exception in DB.exec() instead of ignoring the potential SQLITE_LOCKED reported via sqlite3_finalize()/reset(), and similar constructs implemented in client code will trigger an exception via either finalize() or reset() or may actively check the result code of Stmt.finalize() for an error.

These changes will apply as of 3.42.1 and 3.43.

EDIT: Larry has convinced me that having finalize() throw is just plain bad, so the above solution will be reworked so that finalize() no longer throws, but reset() will continue to.