Implement SqliteCommand.Prepare

[buybackoff]

Prepare method is not implemented and the prepared statement is not reused (as in System.Data.SQLite), but is disposed on every execution. Currently there is no way to reuse a command like in this example.

In the simplest insert case, it takes almost 20% (of which UTF8 marshalling is 2.4 pp):

I haven't found any discussion in issues about that. It is not hard to implement, but I am not sure if it is already on your TODO list and what design you have in your minds?

[bricelam]

It used to be implemented in early days of the project, but there was one big problem: calling sqlite3_prepare() allocated unmanaged memory that eventually needed to be freed by the caller. Although DbCommand implements IDisposable, it is only by coincidence--Component happened to implement it. We found that many ADO.NET-based technologies do not dispose DbCommand objects, which lead to memory leaks. Because of this, we decided to not allocate any unmanaged memory that would be tied to the lifetime of the DbCommand object. See also issue #56.

[buybackoff]

Why not using finalizers for this? Given that SQLite does not support stored procedures, prepared statements functionality is the only opportunity and performance cost is quite substantial.

[bricelam]

@buybackoff Oh I wasn't pushing back on implementing the feature in some other way, I was just giving some history.

[buybackoff]

@bricelam so will you add this back some day?

[rowanmiller]

Moving to the backlog as we do want to keep this around to consider implementing in the future

[roji]

@bricelam regarding your comment on disposal and memory looks above...

Isn't the idea of preparing of a prepared command (almost) always imply, by definition, some external resource being allocated which later has to be freed? In the case of a remote database server like PostgreSQL, resources on the server are allocated when a command is prepared (that's what makes the speedup possible), in sqlite it's in unmanaged memory in the same process, but it's always going to be there...

Also, I understand historically in .NET DbCommands aren't always disposed. But why is that important in a discussion of EFCore using prepared statements? As long as you take care to always dispose your commands inside EFCore you should be fine, should you?

[bricelam]

Yes, unmanaged memory is allocated, but in the current implementation it isn't done until you call execute. We consider the memory to be owned by the DbDataReader. As part of implementing this issue, it would need to be owned by either the DbCommand or DbConnection.

historically in .NET DbCommands aren't always disposed

Our team maintains this principle; we still consider the fact that DbCommand implements IDisposable to be a coincidence and not part of the contract. We dispose in EF to be good citizens, but we don't require users of this provider to.

[roji]

@bricelam I understand what you're saying, but at the end of the day isn't this a purely EFCore-internal issue, i.e. that only touches upon how EFCore manages its DbCommands? If you implemented command preparation, you'd just have to make sure that you guys dispose of command everywhere, without caring whether the rest of the .NET world does so or not, no?

[bricelam]

Isn't this a purely EFCore-internal issue?

No, this particular issue is about adding a general, ADO.NET-friendly way of using prepared statements in Microsoft.Data.Sqlite.

[roji]

My bad, I confused this with dotnet/efcore#5459.

FYI I had a similar problem with un-prepared commands in Npgsql with EF6. At some point Npgsql required you to dispose of a command, otherwise some internal resource was leaked. This caused an issue with EF6 which, as you say, does not dispose of commands. The solution I ended up adopting was to release said resource when a command's connection was closed. You may be able to do something similar with Sqlite's prepared statements, limiting any potential leakage to a connection's open lifetime (of course Dispose could still be used on a command for earlier deallocation).

[buybackoff]

Related issue is byte[] allocation for UTF8 in MarshalEx. Without prepared statements, any SQL command always allocates. This could be mitigated using a buffer pool, or the string marshalling logic could be changed to first call Encoding.GetNumberOfBytes and then to call GetBytes overload that writes directly to a pointer. In my tests that was one of the main garbage generators. (There is also string concatenation in param prefix check). I do not write strings columns now, but this could affect all cases with strings as well, not just SQL statements.

[bricelam]

@roji Awesome! That was our general thinking of how we'd implement this issue. It's good to hear it's been validated. I especially like the idea of freeing the memory sooner if the command does get disposed.

[bricelam]

@buybackoff Can you create a new issue on string marshaling? Those sound like options worth exploring. We're also hoping for dotnet/corefx#7804.

[buybackoff]

Was going to do so, just received an email from this thread and replied. Even when Core has it (any ETA? RC42 maybe... :) ), .NET 4.5/6 will have to deal with it anyway.

[AlexanderTaeschner]

I'm just trying to implement the proposed solution of @roji. If I prepare the statements before the execution, I have a problem with such a statement: connection.ExecuteNonQuery("CREATE TABLE Data (Value); INSERT INTO Data VALUES (0);");, since the call to sqlite3_prepare_v2 fails for the INSERT due to the not yet existing table. How should the library react here?

[bricelam]

A few thoughts:

• Maybe Prepare should just compile the first statement in the batch for now
• If errors like this are encountered, it could fallback to the current behavior (no-op and compile during Execute)
• Let Prepare throw in this case (but ensure Execute still works)

[bricelam]

Re-opening. We had to back this out due to a memory leak. We need to investigate and fix it before merging again.

[AlexanderTaeschner]

I am trying to reimplement Prepare and were able to harden it further. But I still see flaky test failures in EntityFramework.
My current impression is, that there could be a concurrency problem in the test suite, since I see test failures when using Run All, but when I afterwards run the failing cases separately they succeed (which makes debugging almost impossible).
For example the following loop in SqliteConnection leads to intermittent ArgumentOutOfRange exceptions:

            for (var i = _statements.Count - 1; i >= 0; i--)
            {
                if (!_statements[i].TryGetTarget(out var item) || item == stmt)
                {
                    _statements.RemoveAt(i);
                }
            }

which seems for me only possible if _statements is changed by another thread while being in the loop.

The current version can be found at
https://github.com/AlexanderTaeschner/Microsoft.Data.Sqlite/tree/Issue235v2
(The last Run All of the EF test suite succeeded without further errors (only one ArgumentOutOfRange in a skipped test)).

Should I send a pull request for this?
The main changes to the last version are:

• the connection is explicitly closed and with debug builds throws an exception, so that the existance of undisposed statements are indicated
• in ExecuteReader statements are disposed before binding related exceptions are thrown, so that the connection can be closed safely afterwards
• disallow a second open data reader or changes to CommandText or Connection while an reader is open
• since the disposal of commands did not always dispose all statements in the EF test suite (only intermittent failures which could not be debugged),
  added a list of all statements associated to a connection and dispose them excpilictly before trying to close the connection

[bricelam]

Yes, feel free to send a PR; we can close mine.

[bricelam]

I got to the bottom of the issue we were seeing. If a connection got finalized before its readers, the connection was left open. Updating to use sqlite3_close_v2 (in #386) has resolved the issue. This feature can move forward.