Re: [boost] Review of Boost.MySql

On Tue, 10 May 2022 at 22:33, Marcelo Zimbres Silva via Boost < boost@lists.boost.org> wrote:

Hi, here is my review.

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

...

https://anarthal.github.io/mysql/mysql/ref/boost__mysql__resultset/async_rea...

...

The handler signature for this operation is void(boost::mysql::error_code, std::vectorboost::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_qu...

...

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.

I seem to remember this being commented on during the development of the library. It is worth remembering that because this is MySQL, nothing can be done with the connection object until the resultset has been fully read. In effect, the resultset can be thought of as a modal extension to the connection. Causing it to run on a different executor would mean that calls to the resultset would need to be marshalled to the connection's executor in any case, and the opposite in reverse. This may be of questionable value given the limitation of the underlying protocol. This arguably strengthens the case for the use of a connection pool, since without one, as you say, frequent queries will have setup overhead. The point about preferring not to return IO objects by value is a separate stylistic issue, on which I offer no opinion. However it is worth noting that Asio itself does have a form of the ip::tcp::acceptor::async_accept initiation function whose completion handler is passed a socket: https://www.boost.org/doc/libs/1_79_0/doc/html/boost_asio/reference/basic_so...

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

...

https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee...

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/13d3615464f41222052d9ad8fa0c86bcdddee...

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

users could do

boost::mysql::row 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/13d3615464f41222052d9ad8fa0c86bcdddee...

I expect this function to be O(1).

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

...

https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_std_optio...

...

https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_optional....

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

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost