Re: Boost MySQL Review Starts Today

From: Alan de Freitas (alandefreitas_at_[hidden])
Date: 2022-05-13 02:24:19

Hi! Here's my review.

Boost.MySql is an impressive library. I would like to thank Rubén for that.
It implements the complete protocol from the ground up with async
functionalities and certainly involved a lot of work. The documentation is
extensive, goal-oriented, and helpful. The async operations support
cancellation and the examples with coroutines are beautiful. During this
review, I was impressed at how easy it works and I've noticed a few points
in the API that have also been brought up in previous reviews and could be
improved. Some other issues could also be better highlighted in the
documentation, which would avoid many problems. The overall recommendation
in this review is acceptance conditional on some fixes.

## Value

> Will the library bring additional out-of-the-box utility to Boost?

The library is very good news considering our recent discussions about the
future of Boost, where providing more protocol implementations comes up
frequently. I wish more people would make this kind of contribution.

> What is your evaluation of the potential usefulness of the library?

Others have questioned the benefit of the library when compared to sqlpp11
or any wrapper around the C API. The main difference is other libraries are
high-level but this is a discussion still worth having from the point of
view of users. I was thinking about the transition cost from Boost.MySQL to
any other SQL database, since many applications have the
requirement/necessity of allowing different SQL databases. In sqlpp11, the
user can just change the backend. The user could use Boost.MySQL as an
sqlpp11 backend and that would have the same effect. However, I think Rubén
mentioned this is not possible at some point. I'm not sure whether this is
just for the async functionality. In the same line, I wonder if a library
for Postgres or Sqlite would be possible with a similar API, which could
also solve the problem, although I'm not sure anyone would be willing to
implement that. If we did, we could have the convenience of sqlpp11 and the
ASIO async functionalities of Boost.Mysql for other DBs.

The library really provides ease-of-use, when we consider what it provides
and how low-level it is. However, unlike in other libraries like
Boost.Beast, Boost.MySql users might not be sold into the Asio way of doing
things. Applications that require access to databases might be making
sparse database requests where the Asio asynchronous model is not as
useful. Highlighting these differences in the docs is important. Asio takes
some time to learn, and I guess for a user not used to Asio, already
understanding Asio does not sound like the ease of use. The docs could
focus on the protocol before moving on to the asynchronous functionality.

I'm also a little worried about the maintainability of the library and
protocol changes and how this could impact boost as a whole. Should we
really announce it as compatible with MariaDB? What direction would the
library take if they diverge? How often does the protocol change or is
extended? Is Ruben going to stick around if the protocol changes? How hard
would it be for someone to understand the code and implement extensions?
Can a user be sure it's always going to provide the same features and be as
reliable as the C API? I don't have the answer to these questions, but it's
something that got me wondering. I guess this kind of question is going to
come up for any library that is related to a protocol.

I don't know if the name "MySql" can be used for the library, as it belongs
to Oracle. I'm not saying it can't. I'm really saying I don't know. I'm not
a lawyer and I don't understand the implications here. But this should be
considered, investigated, and evidence should be provided. The library is
also compatible with MariaDB and the name "MySql" might not reflect that.
Maybe there's a small probability it might be compatible with some other
similar DB protocol derived from MySql in the future?

As others have mentioned, the protocol is strictly sequential for a single
connection, and this might have some implications for the asynchronous
operations the library provides.

- No two asynchronous MySql query reads can happen concurrently. While this
still has value among other Asio operations, like a server that needs the
DB eventually, the user needs to be careful about that. Maybe it would be
safer if all MySql operations were on some special kind of strand. Or maybe
the library could provide some "mysql::transaction_strand" functionality to
help ensure this invariant for individual queries in the future.
- A second implication is that some applications might find the
asynchronous functionalities in Boost.Mysql not as useful as asynchronous
functionalities in other protocols, like the ones in Boost.Beast. This
depends on how their applications are structured. Since this is the main
advantage over the C API, these users may question the value of the library
and the documentation should discuss this more explicitly.
- These implications could become irrelevant if the library provides some
kind of functionality to enable a non-blocking mode. I have no idea how the
MySql client achieves that.

## API

> What is your evaluation of the design? Will the choice of API abstraction
model ease the development of software that must talk to a MySQL database?

I like how the API is very clean compared to the C API, even when including
the asynchronous functionality. This would be a reason for using the
library, even if I only used the synchronous functions.

I'm worried about the lack of possibility of reusing memory for the
results, as the interface depends on vector. This is not the usual Asio
pattern. These vectors look even weirder in the asynchronous callbacks:

- People have their containers/buffers and I would assume reading into some
kind of existing row buffer would be the default interface, as is the case
with other Asio read functions. In other words, read_many and read_all
should work more like read_one.
- Not returning these vectors is the common pattern in Asio: the initiating
function receives a buffer for storage and the callback returns how many
elements were read. Note that the buffer size already delimits how many
elements we should read.
- If we return vectors, async operations would need to instantiate the
vector with the custom allocator for the operation. The callback wouldn't
use std::vector<T> then. I would be std::vector<T,
very_long_allocator_name> for every query callback, which is inconvenient.
Using only std::vector<T> in the callback is not only inconvenient but
dangerous because it would be the wrong allocator and everything would need
to be copied.
- One problem I see without the vectors is that each row would still need
to allocate memory internally because of their dynamic size. I don't know
the best solution for that second problem unless we have some sort of
static size row/tuple, which does make sense since the user should know the
number of columns at compile time.

The library provides read_one, read_all, and read_many, but no "read_some".
Besides the implication of a buffer to the initiating function, I would
imagine reading as much as possible and parsing that to be the usual best
strategy in this kind of an operation since we have a stream and we don't
know how much data is available to read yet. Unfortunately, read_one,
read_all, and read_many don't allow that. Unless read_many can already read
less than the max number of rows in intermediary calls, which I don't think
is the intention. It might also be useful to replicate the naming
conventions Asio uses to read some or read all of the data.

If "value" is intended to encapsulate the variant type,
"value::to_variant" seems dangerous. The variant supports lots of types,
which indicates the probability the internal design might need to change to
fit new user demands is high. This could also need to change according to
protocol extensions, and changing any of these types would break the API.
If "value" already represents what it needs to represent,
"value::to_variant" may not need to be exposed unless there's a lot of user
demand for that.

There is also an existential value/reference problem with "value":

- The internal variant type includes a string_view whose string is owned by
the row. This seems kind of dangerous or at least misleading. We have a
reference type that's called "value", while this is more like a const
"field view" or something.
- At the same time, it's not a view either because I assume this is
mutable, and updating numbers would not update the row.
- The other values in the variant are value types, which makes "value" both
a value and a reference type depending on the column type.
- A reference type seems appropriate here because we don't want to copy all
the data when iterating the results, but we need some other pattern for
"value". A const view is especially useful because we might be able to keep
value types for numbers, etc... Encapsulating the variant is also helpful
here.
- Calling a type "value" is a bad indication anyway. Any value is a "value".

As others have mentioned, "value::get_std_optional" is unusual. It may not
look problematic at first but it has a number of problems:

1) We have a dangerous precedent because we could also say we need to
support all other boost/std equivalents in the library, such as the
string_view the library uses at many places.
2) We are including and depending on two libraries when we know almost for
sure the user is only using one of them: If there's no std::optional in the
supported platform, we just need boost::optional. If you are requiring a
C++ standard where std::optional is available, then we just need
std::optional.
3) This is also not very useful if the user has both boost::optional and
std::optional, because the code would still need to be refactored from
get_optional to get_std_optional or vice-versa if the user decides to
change it. This could be solved with some BOOST_MYSQL_USE_STD_OPTIONAL or
BOOST_MYSQL_USE_STD_STRING_VIEW, etc., but this would also invalidate the
distinction.
4) This would make the API inconsistent in case Boost moves to a C++17
minimum at some point in the future, replacing boost classes, as we have
been discussing C++14 lately. Both get_optional and get_std_optional would
return the same type. Also, if std::optional is the *standard* type, it
should not be the one whose function needs clarification with a longer name
"get_std_optional".
5) This doesn't need to be a member function, which indicates it shouldn't
be a member function in this case. The class is internally representing
only one of these and conversions will happen anyway. A user in the >C++17
intersection can easily implement their own to_std_optional free function
outside the class.

If I understood the documentation correctly, it's also problematic to use
an empty optional for both a null value and an invalid result. They are
simply not the same thing. Libraries always differentiate an invalid state
from null in a field. This is especially relevant because the documentation
warns us against using the "is_convertible_to" pattern over "get_optional"
because "is_convertible_to" is unnecessarily slow. By the way, it would be
nice to explain why "// WARNING!! Inefficient, do NOT do" is true. I
wouldn't expect that to cost more than creating the optional value.

The documentation states that the behavior of relational operators may
change in the future. What do other libraries do about these comparisons?
We probably don't want something like PHP, where "20" == 20 so that we need
some kind of "20" === 20 in the future, but the documentation implies we
also don't want "value(std::int64_t(200)) > value(std::uint64_t(200))" to
fail. The library makes the whole thing particularly tricky with number
types because not checking the proper number type in the variant would
return invalid/null even when the number could be stored in a variable of
another number type perfectly fine, which is even worse when both invalid
and null are presented by the same empty optional.

About the "resultset":

- If "resultset" is a table, the "set" part of the name might be a little
misleading in C++ as the results are in a deterministic sequence, even
though this is what MySQL may call a similar concept. However, I imagine
there's no equivalent concept in MySql for this specific
Asio-async-lazy-fetching functionality itself.
- This class is more like a "table", "frame", "vector", "list", or just
"result". However, after all the steps of "connect", "prepare", and
"execute", the name "result" gives the impression that the results are
finally there, but this is just another stream object like a "result
fetcher" or something.
- Since the protocol is strictly sequential, it might be useful to not even
have a resultset class at all. The connection could send the query and have
some read function, like the usual read/write functions in Asio can operate
on the same stream.

Minor issues:

- Maybe it's not clear from the examples, but why would the user need
connection::handshake when connection::connection already does it?
- Shouldn't `tcp_ssl_connection::connect` accept an EndpointSequence to be
more like other Asio functions.
- The names "tcp_ssl_prepared_statement", "tcp_ssl_<component>" are quite
repetitive and verbose in the documentation. Maybe "tcp_ssl" could become a
namespace? Isn't that what Asio does for the ssl related objects? Some
names might be reduced if there's no alternative. For instance,
"prepared_statement" might become "statement" since there's no
"unprepared_statement".
- Wouldn't some form of `statement::execute` with variadic args (different
name perhaps) be a simpler alternative to
`statement::execute(boost::mysql::make\_values(...))`. Or maybe
statement::operator(), as Phil suggested. Or some
`statement::execute_with(...)`. In the variadic args, the optional err/info
can go into some specified positions, since they're not ambiguous with the
field variant types. This is a little tricky but definitely useful,
especially if suggestions of customization points are implemented in the
future.

## Implementation

> What is your evaluation of the implementation?

I haven't analyzed the implementation very profoundly. I skimmed through
the source code and couldn't find anything problematic. It would be useful
if the experts could inspect the Asio composed ops more deeply.

CMakeLists.txt:

- I believe the CMakeLists.txt script is not in the format of other boost
libraries in boost/libs so it won't work with the super-project as it is.
- The example/CMakeLists.txt script refers to
BOOST_MYSQL_INTEGRATION_TESTS. I don't think examples can be considered
integration tests.

Examples:

- The examples are very nice. Especially the one with coroutines. They are
also very limited. They are all about the same text queries, which
shouldn't even be used in favor of prepared statements.
- Many examples about continuation styles are not very useful because this
is more of an Asio feature than a library feature. The library feature, so
to speak, is supporting Asio tokens properly. The continuation styles could
be exemplified in the exposition with some small snippets for users not
used to Asio without the documentation losing any value.
- Some examples are simple enough and don't require the reader to know the
rest of the exposition. They are like a quick look into the library. These
could come at the beginning, as in the Asio tutorials and Beast quick look
section.
- The first sync example could be simpler to involve just a hello world
before moving on to other operations.
- The page about the docker container should specify that the username and
password are "root" and "".

Tests:

Some unit tests take a ***very*** long time. Enough to make coffee and a
sandwich. And they seem to not be adding a lot of value in terms of
coverage. For instance, "mysql/test/unit/detail/protocol/date.cpp(72):
info: check '1974- 1-30' has passed" going through all possible dates
multiple times took a long time.

> Did you try to use the library? With what compiler? Did you have any
problems?

No problems at all. GCC 11 and MSVC 19.

## Documentation

> What is your evaluation of the documentation?

The documentation is complete. The main points that differentiate the
library are

- it's a complete rewrite of the protocol,
- it's low-level and
- it's based on Boost.Asio

The documentation should emphasize these points as much as possible,
especially the first one. This should be in the introduction, the
motivation, slogans, logos, and wherever people can see it easily.

The documentation should also provide arguments and evidence that
these design goals are a good idea, as often discussed when the topic is
the value of this library. Why is it worth rewriting the protocol? To what
use cases are such a low-level library useful? Why should a person who
already uses other libraries or the C API care about Asio now? Something
that should also be highlighted is the difference between the library and
other higher-level libraries, in particular, naming names.

Minor issues:

- There's no link in the documentation to the protocol specification. It
would be interesting to know what the reference specification is. Or
whether the protocol was inferred somehow. Is there any chance this
protocol might change? What about divergences between MySql and MariaDB?
How stable is the protocol? For what range of versions does it work? What's
the policy when it changes?
- Some links are broken (for instance, linking to
https://anarthal.github.io/boost-mysql/index.html).
- "All async operations in this library support per-operation
cancellation". It's important to highlight this is per operation in the
Asio sense of an operation but not in the MySql sense of an operation
because the MySql connection is invalid after that.
- "Boost.MySql has been tested with the following versions of MySQL".
MariaDB is not a version of MySql.
- Prepared statements should come first in the examples, to highlight them
as the default pattern.
- The documentation refers to topics that haven't been explained yet. Maybe
"value" could be explained after "row", and "row" could be explained after
"resultset" and "resultset" after "queries".
- The section "Text Queries" is quite small in comparison to other
sections. It could include some examples and snippets like other sections
do.
- "The following completion tokens can be used in any asyncrhonous
operation within Boost.Mysql" -> "Any completion token..."
- "When they fail, they throw a boost::system::system_error exception".
Don't these functions just set the proper error_code, as usual with Asio
and Beast?
- The "MySQL to C++ mapping reference" section should be using a table.
- A small subsection on transactions would be helpful even if there's no
library functionality to help with that.
- The documentation should include some comparisons that are not obvious to
potential users. C/C++ APIs. The advantages of the Asio async model.
Benchmarks if possible.

## Conclusion

> How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent one day in this review. I read all the documentation, ran the
tests, experimented with the examples, and had a reasonable look at the
implementation.

> Are you knowledgeable about the problem domain?

I'm reasonably educated about databases but not an expert. I've been
working a lot with Asio.

> Are there any immediate improvements that could be made after acceptance,
if acceptance should happen?

While it's important to have a general variant type for row values, a
simpler interface for tuples of custom types would be very welcome and
would simplify things by a lot, while also avoiding allocations, since
columns always have the same types. This feature is too obvious since users
almost always know their column types at compile time and this demand is
too recurrent in applications to ignore.

> Do you think the library should be accepted as a Boost library? Be sure
to say this explicitly so that your other comments don't obscure your
overall opinion.

I believe it should be conditionally accepted with the same conditions
stated in other reviews: allowing for memory reuse in the read_* functions
and fixing the "value" type.

Best,

Em ter., 10 de mai. de 2022 Ã s 04:14, Richard Hodges via Boost <
boost_at_[hidden]> escreveu:

> Dear All,
>
> The Boost formal review of the MySQL library starts Today, taking place
> from May 10th, 2022 to May 19th, 2022 (inclusive) - We are starting one day
> after the announced date and extending the period by one day to compensate.
>
> The library is authored by Rubén Pérez Hidalgo (@anarthal in the CppLang
> slack).
>
> Documentation: https://anarthal.github.io/mysql/index.html
> Source: https://github.com/anarthal/mysql/
>
> The library is built on the bedrock of Boost.Asio and provides both
> synchronous and asynchronous client connectors for the MySQL database
> system.
>
> Boost.MySQL is written from the ground up, implementing the entire protocol
> with no external dependencies beyond the Boost library.
> It is compatible with MariaDB.
>
> Connectivity options include TCP, SSL and Unix Sockets.
>
> For async interfaces, examples in the documentation demonstrate full
> compatibility with all Asio completion handler styles, including:
>
> Callbacks:-
> https://anarthal.github.io/mysql/mysql/examples/query_async_callbacks.html
>
> Futures :-
> https://anarthal.github.io/mysql/mysql/examples/query_async_futures.html
>
> Boost.Coroutine :-
> https://anarthal.github.io/mysql/mysql/examples/query_async_coroutines.html
>
> C++20 Coroutines :-
>
> https://anarthal.github.io/mysql/mysql/examples/query_async_coroutinescpp20.html
>
> Rubén has also implemented the Asio protocols for deducing default
> completion token types :-
>
> https://anarthal.github.io/mysql/mysql/examples/default_completion_tokens.html
>
> Reviewing a database connector in depth will require setting up an instance
> of a MySQL database. Fortunately most (all?) Linux distributions carry a
> MySQL and/or MariaDB package. MySQL community edition is available for
> download on all platforms here:
> https://dev.mysql.com/downloads/
>
> Rubén has spent quite some time in order to bring us this library
> candidate. The development process has no doubt been a journey of discovery
> into Asio, its concepts and inner workings. I am sure he has become a fount
> of knowledge along the way.
>
> From a personal perspective, I was very happy to be asked to manage this
> review. I hope it will be the first of many more reviews of libraries that
> tackle business connectivity problems without further dependencies beyond
> Boost, arguably one of the most trusted foundation libraries available.
>
> Please provide in your review information you think is valuable to
> understand your choice to ACCEPT or REJECT including Describe as a
> Boost library. Please be explicit about your decision (ACCEPT or REJECT).
>
> Some other questions you might want to consider answering:
>
>   - Will the library bring additional out-of-the-box utility to Boost?
>   - What is your evaluation of the implementation?
>   - What is your evaluation of the documentation?
>   - Will the choice of API abstraction model ease the development of
> software that must talk to a MySQL database?
>   - Are there any immediate improvements that could be made after
> acceptance, if acceptance should happen?
>   - Did you try to use the library? With which compiler(s)? Did you
>     have any problems?
>   - How much effort did you put into your evaluation? A glance? A quick
>     reading? In-depth study?
>   - Are you knowledgeable about the problem domain?
>
> More information about the Boost Formal Review Process can be found
> at: http://www.boost.org/community/reviews.html
>
> The review is open to anyone who is prepared to put in the work of
> evaluating and reviewing the library. Prior experience in contributing to
> Boost reviews is not a requirement.
>
> Thank you for your efforts in the Boost community. They are very much
> appreciated.
>
> Richard Hodges
> - review manager of the proposed Boost.MySQL library
>
> Rubén is often available on CppLang Slack and of course by email should you
> require any clarification not covered by the documentation, as am I.
>
> --
> Richard Hodges
> hodges.r_at_[hidden]
> tg: @rhodges
> office: [phone]
> mobile: [phone]
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

-- 
Alan Freitas
https://github.com/alandefreitas