Re: Boost MySQL Review

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2022-05-19 18:46:36

My (small) review of Boost MySQL.

First, i want to apologize for this not to be a real review. I just
could not find the time to make a proper one, so this is just some
thoughts that i want to share in the hope they are useful.

========
Are you knowledgeable about the problem domain?
========

Not really. I've used mySQL in the far past, not from C++, and more
recently used postgreSQL (via libpq) and sqlite3. I'm not unfamiliar
with asio, but never used it outside very small projects.

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

A quick reading. My review is solely based on reading the documentation
(which, by the way, is pretty good).

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

That's a great YES. Definitely useful. I lacked such a library a few
years ago (was for postgreSQL, but the issue is the same).

========
What is your evaluation of the implementation?
========

Can't tell, i did not look at it.

========
What is your evaluation of the documentation?
========

Pretty good. I stumbled several times while navigating into a « This
Page Intentionally Left Blank » page. I have no doubt that this will be
fixed. Just a few issues i noted :

* the documentation on encodings/collation could probably be improved.
>From the current state of the documentation, if i have a table with
three columns, one in utf8, the other one in utf-16, and the last one
in iso8859-15, i'm not sure what the returned string_view contents will
be. Is this the raw data ? And if i'm filtering in a WHERE clause, what
is the expected encoding of the values ? The connection encoding or the
field encoding ? Is this different between prepared statements and
litteral queries ? (i have the intuition that it is, from a quick look
at MySQL doc).

* the same goes for date storage and time zone handling. I can live
with the fact that the library only handles UTC dates, if that's
correctly handled. I have had very bad experiences with postgreSQL
trying to be smart and causing more pain than good when handling
timezones. I don't know how bad mysql is in this regard, but I'm pretty
sure there are some pitfalls here as well. While i understand this may
be a bit out of scope for the library, it does provides conversion
between mysql raw data and a c++ time_point, so it should at least make
sure that this time_point is an UTC one. Given that some mysql
date/time types are local or encode a time zone, i expect some issues
to raise here.

========
Will the choice of API abstraction model ease the development of
software that must talk to a MySQL database?
========

I believe yes, however, i have a few concerns with the API :

* the value variant type has a get_optional member (and the
get_std_optional variant). I would prefer that it follows the std
variant get_if pattern. That would also solve the need to handle both
std/boost optional.
* i'm not at ease with the fact that value get and get_optional handles
the same way a NULL value and a type mismatch. I would like to be able
to write get<optional<int>>(), and get an empty optional if value is
NULL, and an exception if value is a float. get_if<optional<int>> would
cause implementation issues, but i think it's just not worth it and we
can live without (a failure in that case means a database schema and
program mistake, so an exception is fine here). Since the user would be
in charge of providing the optional type, that may also solve the issue
of handling both optional types.

* for resultset, the async_read_all/many completion handlers takes a
vector<row>. I would prefer if it were an opaque iterable type (whose
iteration element is a row). This would probably gives far more room
for further optimizations without breaking user code.
* similarly, the row type exposes the fields via a vector of value. I
would prefer if it would itself be iterable (iteration element being a
value). IMHO it should also be "visitable", ie something like
row.for_each(visitor), visitor being a class with overloaded either
operator()(int field_index, int/float/string_view/...) or
operator()(string_view field_name, int/float/string_view/...). That
would also allow gives more room 

* the library define its own « days » type, and explicitly states that
it may not be the same as c++20 days. I may have missed the reason
(they seems equally defined), but i would like it to use c++20 days
when compiled under a c++20 compiler.

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

I think the API can be improved. I think some APIÂ changes would allow
some internal improvements. However, the library in its current state
seems already useful. The overall impression from reading other reviews
is that the quality of the implementation is very good.

My opinion is thus that the library should be ACCEPTED, but it should
be made quite clear that the library is still in a state where some API
changes can occur, and any form of source compatibility may be broken
by update.

And great thanks to Ruben for writing this library, and taking the time
to get it into boost.

Regards,

Julien

Le samedi 14 mai 2022 à 10:58 +0200, Richard Hodges via Boost a écrit :
> Dear All,
> 
> We've had a good response for calls to review this much anticipated
> library, and there is a lot of interest on Reddit:
> https://www.reddit.com/r/cpp/comments/unik91/acceptance_review_for_boostmysql_has_just_started/
> .
> 
> In general the public perception of utility libraries like this in
> Boost
> seems popular, and Boost's emphasis on quality is often mentioned.
> 
> If you are able, I would be more than grateful if you could take a
> few
> hours over this delightful weekend to offer a review. The more eyes
> on this
> project the better, as if it is successful, it will no doubt prompt
> submission of similar libraries targeting other popular database and
> messaging systems.
> 
> The Boost formal review of the MySQL library started on May 10th,
> 2022 and
> will conclude on May 19th, 2022 (inclusive) - In fact, I am able to
> accept
> submissions up until the (GMT) morning of May 20th as I'll be
> traveling on
> May19th.
> 
> 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.
> 
>