simolus3 / drift

Drift is an easy to use, reactive, typesafe persistence library for Dart & Flutter.
https://drift.simonbinder.eu/
MIT License
2.62k stars 364 forks source link

batch not inserting into DB #271

Closed DevonJerothe closed 4 years ago

DevonJerothe commented 4 years ago

So I am currently using my own batching for inserting large lists of data. Now that my use of insertAll is deprecated I wanted to refactor to moors batching. Im not sure if I'm doing something wrong here, can't seem to find much documentation on it.

Here is my original implementation:

Future batchUpdate(String userId, List<api.Video> videos, bool archivedVid,
      DateTime updated) async {
    final _items = <VideoRecordsCompanion>[];
    final _updated = updated;
    for (var video in videos) {
      final _item = VideoRecordsCompanion(
          videoId: Value(video.id),
          userId: Value(userId),
          name: addField<String>(video?.name),
          archived: addField<bool>(archivedVid),
          description: addField<String>(video?.description),
          thumbnail: addField<String>(video?.thumbnail),
          video: addField<String>(video?.video),
          download: addField<String>(video?.download),
          width: addField<int>(video?.width),
          height: addField<int>(video?.height),
          duration: addField<int>(video?.duration?.inSeconds),
          views: addField<int>(video?.views),
          plays: addField<int>(video?.plays),
          dateCreated: addField<DateTime>(video?.dateCreated),
          dateModified: addField<DateTime>(video?.dateModified),
          lastUpdated: Value(DateTime.now()));
      _items.add(_item);
    }
    return transaction(() async {
      await into(videoRecords).insertAll(_items, orReplace: true);
      await cleanVideos(userId, _updated);
    });
  }

This transaction is return by a method inside my DAO file and is called from a Bloc as such:

await GetIt.instance<DatabaseService>()
          .videosDao
          .batchUpdate(user.id, _videos);

using batch I simply change my transaction to this:

return transaction(() async {
      await batch((l){
        l.insertAll(videoRecords, _items, mode: InsertMode.insertOrReplace);
      });
      await cleanVideos(userId, _updated);
    });

Am I missing a step? the original deprecated method of insert is working for me for now.

simolus3 commented 4 years ago

A batch can't be used from inside a transaction, since a batch starts a transaction on its own and we don't support nested transactions at the moment. Attempting to do that should throw an exception though, it definitely shouldn't do nothing. I'll check why that's not the case here.

The real problem, of course, is that now insertAll and cleanVideos can't be executed atomically. Ideally, we should support nested transactions, but I'm not sure if that's possible with sqflite, the underlying library we use for moor_flutter. I would probably suggest to continue using insertAll until a solution exists, maybe I should un-deprecate that method.

ghost commented 4 years ago

Hello @simolus3 , We running into warning issues on flutter analyze using insertAll() method. We are small team of student building app using your impressive package and the overall thing is a graduation project. We had to add a CI/CD tool to enable professional integration and deployment of prototype app. We chose Codemagic, and all builds fail since we added a call to insertAll() as you marked it as deprecated. (but somehow i just learned it by reading above comment?? where can I find the info elsewhere? when I "Go to definition" on IDE, the method is not tagged deprecated, and my local flutter analyze don't say anything)

Can you provide me a way of fixing this? I have to use batchis that right?

Here is codemagic flutter analyze output: image And here is our call to insertAll() : image

simolus3 commented 4 years ago

I "Go to definition" on IDE, the method is not tagged deprecated, and my local flutter analyze don't say anything)

Maybe you have an outdated version of moor on your machine? Try running flutter packages upgrade and see if that changes anything. You can migrate your example to batches by using:

Future insertAll(List<ClientAdresseData> listClientAdresse) async {
  await delete(clientAdresse).go();
  await batch((b) {
    b.insertAll(clientAdresse, listClientAdresse);
  });
}
simolus3 commented 4 years ago

Regarding a better fix for this - maybe we should change the behavior of batch when called in a transaction block, so that

  1. calling batch outside of a transaction implicitly starts a transaction
  2. calling batch inside a transaction re-uses that transaction

This would fix the original problem, since

return transaction(() async {
  await batch((l){
    l.insertAll(videoRecords, _items, mode: InsertMode.insertOrReplace);
  });
  await cleanVideos(userId, _updated);
});

would work as expected then. I don't see a strong reason for always making batch start it's own transaction.

DevonJerothe commented 4 years ago

is there any direct benefit of using batch insertALL verse my current implementation?

simolus3 commented 4 years ago

Not really - InsertStatement.insertAll uses a batch api provided by sqflite internally. The overall idea is that there are fewer messages to send over method channels, so the performance will be better. So InsertStatement.insertAll and batch use the same underlying api. It's similar with moor_ffi, there are fewer statements to prepare, which allows us to cut out some duplicate parsing costs in sqlite.

I wanted to introduce the batch api to make it more explicit how those features work under the hood, and to improve the flexibility of batches in moor (you can mix inserts and updates into a single batch now). I still think that's the right way forward, but batches should work better together with transactions.

DevonJerothe commented 4 years ago

Closed as both methods are working as intended, Thx!