Review of Boost.MySql

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2022-05-10 20:33:00

Hi, here is my review.

1) ================================

> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__resultset/async_read_all.html
> The handler signature for this operation is
> void(boost::mysql::error_code, std::vector<boost::mysql::row>).
> etc.

Having a std::vector in the signature of the completion handler is
unusual in ASIO.  In general, large objects should be passed as
parameters to the initiating function.

Additionally, this operation does not support custom allocators. That
wouldn't be a big problem to me if I could reuse the same memory for
each request but the interface above prevents me from doing so.

It is also a pessimized storage as each row will allocate its own
chunk of memory. A single block of memory would be better.

2) ================================

> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__connection/async_query/overload1.html
> The handler signature for this operation is
> void(boost::mysql::error_code, boost::mysql::resultset<Stream>)

Here the completion handler is returning an io object i.e.
boost::mysql::resultset, which is also unusual as far as I can see.
Wouldn't it make sense for users to specify a different executor for
the resultset? This interface is preventing that.

I prefer having the completion handler return config parameters with
which I can instantiate a boost::mysql::resultset myself in my desired
executor.

3) ================================

> https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/include/boost/mysql/value.hpp#L72

I am not convinced the design of the value class is a good idea.  The
reason is that users know at compile time what types they are
expecting from a given query. That means it would be possible for them
to specify that before doing the query. So instead of doing this

https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/example/query_async_coroutines.cpp#L107

   boost::mysql::row row;
   while (result.async_read_one(row, additional_info, yield[ec]))

users could do

   boost::mysql::row<T1, T2, T3, T3> row;
   while (result.async_read_one(row, additional_info, yield[ec]))

avoiding unnecessary further copies. In principle it would be even
possible to parse directly in a custom data structure, for example,
when storing json string in the database.

4) ================================

> https://anarthal.github.io/mysql/mysql/examples.html

IMO, the examples are too limited and show variation of the same
functionality. For example

      - Text query, async with callbacks
      - Text query, async with futures
      - Text query, async with Boost.Coroutine coroutines
      - Text query, async with C++20 coroutines
      - Default completion tokens

Here you are showing nice Asio features. I am not meaning you should
remove them but if I am not mistaken there is no example that shows

  - How to keep long lasting connections open (specially for ssl).
  - Timers for resolve, connect, read and write.
  - Synchronized writes.
  - Healthy checks, failover etc.

I estimate that to address these points users will be required to
write further 500 - 1000 LOC on top of your library. Otherwise they
will be stuck on underperforming short lived connections.

It would be helpful for users to have a starting point to begin with.

There some minor things I have also noticed:

5) ================================

> https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee537/include/boost/mysql/impl/error.hpp#L35

I expect this function to be O(1).

6) ================================

> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_std_optional.html
> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_optional.html

Sounds also unusual to have two member functions for the different
versions of optional. I suggest using the boost version until we
transition to C++17.

7) ================================

> https://anarthal.github.io/mysql/mysql/ref/boost__mysql__errc.html

errc should be conditions and not error AFAICS.

============= Review End ================

I vote for conditional acceptance. Points 1, 2, 3 and 4 should be
addressed before acceptance. My acceptance criteria sums up to

  1) Remove functionality that provides low performance. With
coroutines it will be easy for users to loop on async_query to create
a std::vector of rows.

  2) No IO object should be returned as a parameter of the completion handler.

  3) Review the value type. Can it be improved?

  4) Improve examples.

Regards,
Marcelo