close
The Wayback Machine - https://web.archive.org/web/20200903034718/https://github.com/dotnet/efcore/issues/21090
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Microsoft.Data.Sqlite: Add way to provide underlying SQLitePCL connection for SqliteConnection #21090

Open
wanton7 opened this issue May 31, 2020 · 12 comments

Comments

@wanton7
Copy link

@wanton7 wanton7 commented May 31, 2020

I was looking at SQLite docs and it seems single connection supports multithreading in serialized mode. From what I read here default mode for included SQLite is serialized. So you could just use single connection for lifetime of your app if you don't care about sharing PRAGMAs etc.
But current SqliteConnection implementation doesn't support providing your own underlying SQLite connection.

Add constructor that allows providing your own SQLitePCL connection.

@wanton7 wanton7 changed the title Microsoft.Data.Sqlite: Add way provide underlying SQLitePCL connection for SqliteConnection Microsoft.Data.Sqlite: Add way to provide underlying SQLitePCL connection for SqliteConnection Jun 1, 2020
@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Jun 1, 2020

/cc @bricelam

@bricelam
Copy link
Member

@bricelam bricelam commented Jun 1, 2020

I don’t think SqliteConnection is thread safe (even if the underlying sqlite handle is). But I’d need to investigate.

@bricelam
Copy link
Member

@bricelam bricelam commented Jun 1, 2020

But yes, going from sqlite to SqliteConnection seems useful. (Same for sqlite_stmt and sqlite_blob)

@wanton7
Copy link
Author

@wanton7 wanton7 commented Jun 1, 2020

That's why it would be great if you could constuct new SqliteConnection from raw SQLite connection. This way SqliteConnection class wouldn't need to be thread safe.

@bricelam
Copy link
Member

@bricelam bricelam commented Jun 1, 2020

Although a static method like FromHandle might be better to avoid overwhelming the constructor

@ajcvickers ajcvickers added this to the Backlog milestone Jun 5, 2020
@KaloyanIT
Copy link
Contributor

@KaloyanIT KaloyanIT commented Aug 3, 2020

I'm interested in contributing. If I understand correctly, I should add a static method FromHandle, that can create a SqliteConnection from raw SQLite connection. Could someone give me more details, thank you!

@bricelam
Copy link
Member

@bricelam bricelam commented Aug 10, 2020

@KaloyanIT Yep. I imagine it would do something like this:

var connection = new SqliteConnection();
connection.ConnectionOptions = new SqliteConnectionStringBuilder
{
    DataSource = sqlite3_db_filename(handle),
    Mode = sqlite3_db_readonly(db, "main") == 0 ? default : SqliteOpenMode.ReadOnly,
    // Cache = ???
};
connection._connectionString = ConnectionOptions.ToString();
connection._db = handle;
connection._state = ConnectionState.Open;

return connection;

We might also need to avoid closing the handle when the SqliteConnection is disposed...

@bricelam
Copy link
Member

@bricelam bricelam commented Aug 10, 2020

Hmm, I don't see a way to tell if the database is using a shared cache. We may need to just trust the user if they try to use IsolationLevel.ReadUncommitted.

@groege
Copy link

@groege groege commented Aug 28, 2020

@bricelam
I'm sure there is a reason why serialized instead of multithreaded mode is in use - can you give some information why this is?
As I see it https://sqlite.org/threadsafe.html

Single-thread. In this mode, all mutexes are disabled and SQLite is unsafe to use in more than a single thread at once.

Multi-thread. In this mode, SQLite can be safely used by multiple threads provided that no single database connection is used simultaneously in two or more threads.
==> As I see it we should use this because it pretty much behaves the way I'd expect ef core to behave...
(so one connection for each db context)

Serialized. In serialized mode, SQLite can be safely used by multiple threads with no restriction.

Would it not be a lot better for performance to be able to use sqlite in multithreaded mode for read and write?
I was wondering why a simple statement took very long (22 rows in total and I only selected one), then realized it is because there is a background process that synchronized the data with the server...
So any app (in my case Android) that does some background sync with the db will strongly impact ui performance...

@bricelam
Copy link
Member

@bricelam bricelam commented Aug 28, 2020

@groege When I've thought about this in the past, I was a little concerned about creating the connection on one thread but executing commands on another. (The connection pool #13837 would do this.) I think this is ok so long as it's not used simultaneous on different threads.

Can you submit a new issue? Performance is one of the themes for 6.0, and it would be good to investigate whether we can safely pass SQLITE_OPEN_NOMUTEX during SqliteConnection.Open()

@groege
Copy link

@groege groege commented Aug 31, 2020

@groege When I've thought about this in the past, I was a little concerned about creating the connection on one thread but executing commands on another. (The connection pool #13837 would do this.) I think this is ok so long as it's not used simultaneous on different threads.

Can you submit a new issue? Performance is one of the themes for 6.0, and it would be good to investigate whether we can safely pass SQLITE_OPEN_NOMUTEX during SqliteConnection.Open()

@bricelam Do you mean a general perf issue or about making sqlite multithreaded? (maybe compile it with all options enabled so the dev can decide which mode to use via modelbuilder, etc...)

@bricelam
Copy link
Member

@bricelam bricelam commented Aug 31, 2020

Filed #22330.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.