Skip to content

Add SQLite session support #300

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

rodydavis
Copy link
Contributor

@rodydavis rodydavis commented Apr 17, 2025

  • FFI implementation.
  • Check resource leaks on FFI
  • WASM implementation
  • Allow accessing old/new/conflicting values in conflict callback

@simolus3
Copy link
Owner

simolus3 commented Apr 18, 2025

Thanks! I'll take a look at WASM support tomorrow. I've changed some things to make the API feel a bit more native to Dart (e.g. it makes no sense to me to have enable() and disable() methods, we can just have a bool enabled). I've also added APIs to iterate over a changeset.

Regarding the implementation, I think we should not add bindings for stuff we don't need (yet). This includes the conflict handling / rebasing API that is still marked as experimental. Another concern is avoiding resource leaks, I've added native finalizers around the session / changeset classes to make sure they get deallocated natively when they're no longer referenced in Dart. I definitely need to check this again more carefully though.
Something we definitely need to check before merging this is that package:sqlite3 must continue to work against sqlite3 builds without the session extension (as long as the session methods are not actually used of course).

I've also started writing tests for this, but I couldn't get the extension to work. This test fails due to the session still being empty after making a change. Do you know what could be up with that? It's entirely possible I broke something refactoring the native implementation, but the _create and _attach calls look good to me so I don't understand what's going wrong here.

test('isEmpty', () {
  final database = sqlite3.openInMemory();
  final session = Session(database);
  expect(session.isEmpty, isTrue);
  expect(session.isNotEmpty, isFalse);

  // Change without attaching session
  database.execute('INSERT INTO entries DEFAULT VALUES;');
  expect(session.isEmpty, isTrue);

  session.attach();
  database.execute('INSERT INTO entries VALUES (?);', ['my first entry']);

  expect(session.isEmpty, isFalse); // fails!
});

@rodydavis
Copy link
Contributor Author

rodydavis commented Apr 19, 2025

Thanks! I'll take a look at WASM support tomorrow. I've changed some things to make the API feel a bit more native to Dart (e.g. it makes no sense to me to have enable() and disable() methods, we can just have a bool enabled). I've also added APIs to iterate over a changeset.

I was trying to expose the Node.js Session abstraction but totally fine with those changes. I would prefer as close to the C api as possible.

Regarding the implementation, I think we should not add bindings for stuff we don't need (yet). This includes the conflict handling / rebasing API that is still marked as experimental.

I was debating including them but totally fine not having them until they are stable!

Another concern is avoiding resource leaks, I've added native finalizers around the session / changeset classes to make sure they get deallocated natively when they're no longer referenced in Dart.

Good call, I wasn't sure how to implement that with what I was seeing in the code base.

I definitely need to check this again more carefully though.
Something we definitely need to check before merging this is that package:sqlite3 must continue to work against sqlite3 builds without the session extension (as long as the session methods are not actually used of course).

As far as my testing, there should be no reason it would not work with any version of sqlite that does not have the proper compile flags. You for sure need them to call the methods though. Probably could check the config object for the two flags if needed too.

I've also started writing tests for this, but I couldn't get the extension to work. This test fails due to the session still being empty after making a change. Do you know what could be up with that? It's entirely possible I broke something refactoring the native implementation, but the _create and _attach calls look good to me so I don't understand what's going wrong here.

This tripped me up when first working with it and the issue is the table must have a primary key to be tracked! It cannot be a temp or virtual table. The primary key must also be deterministic (like INTEGER or TEXT).

CREATE TABLE IF NOT EXISTS my_table (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT);
INSERT INTO my_table (name) VALUES ('1'), ('2'), ('3');

"The session extension only works with tables that have a declared PRIMARY KEY. The PRIMARY KEY of a table may be an INTEGER PRIMARY KEY (rowid alias) or an external PRIMARY KEY."
https://sqlite.org/sessionintro.html

Also these 2 flags must be set when compiling:

-DSQLITE_ENABLE_SESSION -DSQLITE_ENABLE_PREUPDATE_HOOK

The official WASM build does have these on by default and works pretty well. Having them in the flutter_sqlite_libs would be amazing.

@rodydavis
Copy link
Contributor Author

Also for the isEmpty test:

"By default, this function always returns 0. For it to return a useful result, the sqlite3_session object must have been configured to enable this API using sqlite3session_object_config() with the SQLITE_SESSION_OBJCONFIG_SIZE verb."
https://sqlite.org/session/sqlite3session_changeset_size.html

That got me too and I thought it was a bug till I looked at the docs!

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

Successfully merging this pull request may close these issues.

2 participants