Re: [review] [sqlite]

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2024-11-20 10:16:46

>> 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.

Isn't this what you have right now?

>>
>> 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.

You got me in this one - I didn't know this was legal. I still think
this should be an error. If you actually expect different types in a
single field, an appropriate representation (like sqlite::field or a
variant) should be used.

>> 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)

If that's the case, I think it's best to just don't use a throwing
destructor, and don't communicate the error anyway.

>> 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

I know the cast is well-formed, but the target object doesn't
technically exist (as opposed to a member subobject, which starts
existing when the parent object is created).

Thanks,
Ruben.