On Wed, Nov 20, 2024 at 2:25 AM Ruben Perez
Hi all,
This is my review of the proposed Boost.Sqlite. First of all, thanks Klemens for submitting the library, and Richard for managing the review.
Thank you for investing the time to write a review.
- Will the library bring additional out-of-the-box utility to Boost?
I think it will. SQLite is very popular, and many people will likely benefit from the library.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
I had two use cases in mind: regular SQL use, and virtual tables as a way to expose in-memory data structures. I've implemented both. My experiences with them follow.
To try regular SQL use, I adapted an existing example I used to have in Boost.MySQL. It simulates an order management system for an online store, with a command-line interface. It's intentionally simplistic. I know that SQLite is more targeted towards resource-constrained systems, rather than web applications, but it's been enough to have a taste of how using the API looks like. For reference, the resulting code is here: https://gist.github.com/anarthal/40c018cc4d133d0c9a082814d99d2a7a
I've used connection::prepare, connection::query, connection::execute, statement, static_resultset and transaction. I've found the API mostly satisfactory. Some gotchas:
1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following:
sqlite::static_resultset
get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute ({order_id}); } This is undefined behavior (as stated in the docs), and needs to be rewritten as:
sqlite::static_resultset
get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute ({order_id}); } The problem being that both sqlite::static_resultset and sqlite::statement are implemented in terms of a sqlite3_statement*. The second snippet makes the static_resultset own the statement handle, while the first version makes it a view. This problem will probably appear much more frequently if you're using error codes instead of exceptions.
IMO the problem arises due to a mismatch between the SQLite and the Boost.Sqlite object model regarding statements and resultsets. Resultsets don't exist in the C API, and thus may become problematic in the C++ API. Making a type sometimes owning and sometimes non-owning can be misleading.
Do you think this could be solved with ref-qualified overloads, i.e. .execute() && would transfer ownership, while .execute() & would not? That would seem intuitive to me. <snip>
4. In the static interface, type mismatches are silent. If one of your field types doesn't match what your query returns, the library doesn't emit any error and silently sets the field to its default value. While this matches what the sqlite3_column_xxxx functions do, I think it'd be more helpful to emit an error. Such type mismatches are likely due to programming errors, and erroring helps to detect them soon. For instance, Boost.MySQL does this by checking metadata (you can probably use sqlite3_column_type to perform the check).
Note that this reflects sqlite's behaviour, its tables aren't strictly typed. That is, a column might be defined as INT, but a user could insert a "foobar" in it. And then it's going to be a string. While odd, that's not a programming, but user error. I think adding a strict mode for the static_resultset would be the way to go, i.e. an opt in similar to strict tables in sqlite. sqlite only added type checking as an option in 2021 with strict tables. <snip>
- Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database?
The API simplifies regular SQL usage, as explained above. A couple of comments:
1. The virtual table infrastructure uses a macro (BOOST_SQLITE_NO_VIRTUAL) to conditionally make functions virtual. I think this is a potential problem in the long-run. If virtualization is not required, appropriate compile-time checks should be performed, instead. 2. sqlite::transaction features a throwing destructor, which I find surprising.
That's for backwards compatibility, ROLLBACK won't fail these days, but it did in older versions. So in practice it won't throw, unless you're using some ancient sqlite instance. (https://www.sqlite.org/lang_transaction.html)
- What is your evaluation of the implementation?
I've only peaked at it in particular sites, and have the following comments:
1. transaction::commit throwing and non-throwing overloads seem to have different behavior on error. One will call rollback() and the other one won't. This case doesn't look to be covered by unit tests. 2. The virtual table infrastructure may be invoking undefined behavior by violating strict aliasing, casting sqlite3_value* into sqlite::value via a reinterpret_cast. Some conversations have happened in the Slack workspace about it, with conflicting opinions whether this is actual undefined behavior or not.
It's not, the sqlite::value class has a standard-layout class and the element it's cast from is the first in the class. https://eel.is/c++draft/basic.compound#5.3 <snip>
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've spent around 12h doing inspecting the library, building the examples, writing the online store simulator, adapting the multi_index.cpp example and writing this review.
I really appreciate the time invested, thank you!
- Are you knowledgeable about the problem domain?
I've had mild SQLite experience in the past, mainly in prototypes. I didn't have prior experience with virtual tables. I have experience with SQL in general, and am the author of Boost.MySQL.
Final decision
Unfortunately, my final recommendation is to REJECT Boost.Sqlite in its current form. I think the concept is interesting and could benefit many people, but the library still requires some work before it gets into Boost. The documentation needs a lot of work. Getting some field experience would also be beneficial.
Thanks again Klemens and Richard for your effort. I'd love to see the library being proposed again in the future.