Transaction based sqlite3 inserts

[sriramster]

Hi @Karsten1987 in continuation with the discussion we've had about using trasactions.

This is a very high level implementation of how the code looks. Plaes have a look and let me know your thoughts.

https://gist.github.com/sriramster/2f9bdd620730855e6172c7136b56d6fc

Sriram

[Karsten1987]

Looks like a good start. I'll give it a try ASAP and see if I can extract some meaningful numbers and stats.

For the time being, a few remarks. Also please make sure that apart from colcon build ... you also run colcon test to address linters and style guides throughout your code.

[sriramster]

Hi @Karsten1987 Thank you for the review and your time. I've updated some pieces based on your comments.

Let me know if you've a test script, and I'd be more than happy to test and share the numbers with you.

[sriramster]

Hi @Karsten1987 the branch compiles with master. Could you please have a look?

[sriramster]

Hi @Karsten1987 Have you had time to look at the above patch set?

[sriramster]

Hi Karsten,

Just trying to be on your github notifications, for this pull request. Have you had time to review this? Do I need to rework anywhere?

[Karsten1987]

I'll revisit it by the end of today. For the time being, could you rebase your PR on top of master? It looks like there are conflicting files.

[sriramster]

@Karsten1987 done. Rebased.

[Karsten1987]

A few comments.
We have to polish this a bit more before we can merge though. I am thinking about the ifdef and the hardcoded 10000 here.

[sriramster]

Hi @Karsten1987

Fixed the points mentioned above, except for few things.

• Warning user about active transaction. Do we need this? Since transactions are controlled by the plugin layer. And a user can not interact with a transaction, active or inactive. But it might be a point to debug if this why you intend to have it. I'll add it.
• Adding the transaction number as a part of storage options, makes us change the open() API, from the base interfaces. Do you mind if I overload the function open() with a different control flow? The problem with this approach is, separation of reader and writer flow control APIs from class loading to sqlite changes.

Please let me know your thoughts on this. I'll update the review accordingly

Sorry for the delay, could'nt work over the weekends.

[Karsten1987]

There are still a few outstanding comments.

Warning user about active transaction. Do we need this? Since transactions are controlled by the plugin layer. And a user can not interact with a transaction, active or inactive. But it might be a point to debug if this why you intend to have it. I'll add it.

I see your point here. If the transaction handling is completely handled internally, we don't need to do that (previously you had the activate/deactivate transition functions part of the public interface). We might however log to DEBUG instead.

Adding the transaction number as a part of storage options, makes us change the open() API, from the base interfaces. Do you mind if I overload the function open() with a different control flow? The problem with this approach is, separation of reader and writer flow control APIs from class loading to sqlite changes.

We might want to change the io interface then with a struct like rosbag2_storage::IOOptions or rosbag2_storage::IOConfig which includes the existing IOFlags and can then be extended with something like a chunk_size or any generic term which fits to the transactions size.

[sriramster]

Hi @Karsten1987 I've cleaned up a bit focussing your review comments. And working on the chunk_size/transaction_size as an option. Kind of stuck at point, just updating you. Have'nt given up yet here.

[sriramster]

Hi @Karsten1987 I've pushed in the changes as suggested by you. Could you please have a look and let me know if they fit into the model.

[Karsten1987]

@sriramster thanks a lot for not giving up on this ;-)

Up front: This PR has to be rebased once more as there're some merge conflicts.

I am seeing two potential ways on how to integrate a generic chunk_size option.

• The SQLite plugin (in fact, every plugin) gets the chunk size passed in when being opened as read_write. It's then in the plugin's responsibility to deal appropriately with it. The open_read_write() function has therefore to be enhanced. I think this is what you've currently implemented in this PR.
• We deal with chunk sizes exclusively in the rosbag2_cpp API. That is, we keep implement a cache in the c++ API which holds all incoming messages until it's succeeding the chunk size and only then asks the storage API to write it to disk. We therefore have to enhance the writer interface with an additional write(std::vector<SerializedBagMessage>) method which writes multiple messages at once.

The first option would give more leeway to each individual plugin to come up with an optimal way of dealing with chunk sizes. The second one, however, eases the burden of each plugin as the logic has to be implemented only once and is therefore less error-prone.

I believe I am tending towards the second option here as I imagine that we could technically also leverage sqlite3 transaction when writing multiple messages at once. I further think that keeping the serialized bag messages in a vector within rosbag2_cpp does not introduce a big computation overhead.

So essentially what I would imagine could work nicely:

// in rosbag2_cpp

rosbag2_cpp::Writer::write(
  std::shared_ptr<rosbag2_storage::SerializedBagMessage> message)
{
...
// There might exist an optimal data type for this,
// but at first a `std::vector` might do
cache.push_back(message);
if (cache.size() >= storage_options_.chunk_size) {
  // new signature of write in BaseWriterInterface to write multiple
  storage_->write(cache);
}

// in SqliteStorage plugin

void SqliteStorage::write(
  std::vector<std::shared_ptr<const rosbag2_storage::SerializedBagMessage> & messages)
{
  start_sqlite_transaction();
  for (auto & serialized_bag_message : messages) {
     write(serialized_bag_message);  // call to normal write
  } 
  commit_transaction();
}

Does this make sense what I writing here? What do you think?

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros2-tooling-wg-next-meeting/12545/1

[sriramster]

@Karsten1987 I'm reworking this. Been a bit busy at work. Will update you at the earliest, sorry about the delay.

[sriramster]

Hi @Karsten1987 I've reworked the patch set.

I'd designed it based on the first method we'd discussed above to make sure the control of transactios is closer to the sqlite plugins only. But in a way, I feel moving it to a higher layer would give the flexibility of working with transactions and plugins per-say. Not all the plugins might have transactions that is one reason why this was sqlite3 specific. Just stating thought process behind the idea.

Pleae let me know if this patch set looks ok.
Thanks for the time.

[sriramster]

Hi @Karsten1987 friendly reminder, have you had time to look into this?

[sriramster]

Hi @Karsten1987 remind you again on this. :-)

[Karsten1987]

thanks @sriramster, I like that approach.

Can you please address all outstanding comments? I've added a few new ones, but there are a bunch of previously mentioned comments as well.
Once you've addressed all of them, please make sure to run colcon build and colcon test in your rosbag2 workspace to address all other compilation and style/test failures.

[Karsten1987]

@sriramster did you have a chance to look at the review? I would love to have this PR merged for the Foxy release.

[Karsten1987]

new round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

In order to verify that the transactions are used correctly, I had to expose the max_cache_size through the ros2cli interface in bac3558.
I tried to put that step actually in a separate PR, but it turned out that the sqlite3 statements were wrongly placed. That's corrected in 83042cc.

With the option being exposed in the command line interface, I'll have to enhance this PR with some end-to-end tests. I could confirm locally though that the transactions are working currently with the current set changes.

[Karsten1987]

CI (with cache end to end test):

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

• Linux
• Linux-aarch64
• macOS
• Windows

[zmichaels11]

I agree with using 0 as default since it will be more consistent with max_bagfile_size = 0 also being default.

[sriramster]

@Karsten1987 Thanks for considering this. I see a lot of activity here, hope this gets merged soon.

[piraka9011]

LGTM, pending Green CI and Zach's comments addressed.

[Karsten1987]

one more round:

• Linux
• Linux-aarch64
• macOS
• Windows