Re: [review] [sqlite]

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-11-19 21:25:32

On Wed, Nov 20, 2024 at 2:25 AM Ruben Perez <rubenperez038_at_[hidden]> wrote:

> 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<order_with_items>
> get_order_with_items(std::int64_t order_id)
> {
>     sqlite::statement stmt = conn.prepare("SELECT ...");
>     return conn.execute<order_with_items>({order_id});
> }
>
> This is undefined behavior (as stated in the docs), and needs to be
> rewritten as:
>
> sqlite::static_resultset<order_with_items>
> get_order_with_items(std::int64_t order_id)
> {
>     return conn.prepare("SELECT ...")
>         .execute<order_with_items>({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.
>
>