My review of Boost Mysql I'll loosely go by the guidelines [here](https://www.boost.org/community/reviews.html#Comments) # What is your evaluation of the potential usefulness of the library? Highly useful. In fact, my feeling was mostly "where was this library back in 2016". Back then we had to postpone necessary refactoring with big scalability benefits because I wasn't ready to do the async initiation function myself, or figure out how the mysql native client wants to do asynchrony. # What is your evaluation of the design? It is largely what I'd expect. I might consider dropping the `*read_many` interfaces which has been been subject of discussion (mainly for returning `vector<row>`). I guess they could also be trivially improved by taking a reference to any mutable range of `row` objects instead (obviating the need to specify a count). In my use, I find a "cursor-style" approach to consuming results (reusing the row buffer) the most natural way to async consume result-sets. Hence `read_one()` and perhaps `read_all()` would be the only interfaces I like to see. I'll probably have some more notes below. # What is your evaluation of the implementation? Pleasantly surprised. A good example is when I zoomed in on the "generic execute" algorithm implementation. It looks completely readable, which is quite an accomplishment considering the domain. Little cues picked up elsewhere (e.g. in tests) confirmed the impression. This will positively affect both technical the maintainability and chances of community interest to support that. Also, I think given a clean state of the implementation, the library could serve as a inspiration for people wishing to implement other protocols. # What is your evaluation of the documentation? The documentation gave me a good idea of what the library does and doesn't do. I'm not convinced of the "user-friendliness" angle. That's fine for me. It seems obvious that a library that implements a third-party wire-protocol from scratch is not focusing on high-level abstraction. It is good to manage the expectations in the documentation, though. In particular, a number of features seem more user-hostile. This is felt e.g. in the (lack) of user-defined conversion support and interoperability (see below for concrete examples with e.g. `mysql::value`). I have to mention the statically parameterized class hierarchy. To be clear, I wouldn't want it any other way, but to end-users it does *not* seem user-friendly: they have to template a good part of their DB layer on the chosen stream type if they just want to e.g. optionally support SSL and UNIX domain socket access. Most ecosystems would consider it standard to have a connection-string representation with ditto parser that can dynamically spawn the right implementation types. I also feel the situation can be improved by providing the conventional nested type names (see below) # Did you try to use the library? With what compiler? Did you have any problems? # How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I looked at this library last year around March, and again this week. Both times I spent roughly 4 hours exercising the interfaces and accomplishing typical tasks. This time around I spent also some time reading the docs. I used my own existing databases (MySql 5.7), mainly asynchronously, focusing on the `read_one()` 'cursor` approach, several Stream protocols and simultaneous connections. I tried some DDL tasks and insertions vs. auto-increment fields. I used GCC 10 on linux with CMake. I found I had to add the include path, which doesn't seem mentioned in the documentation: INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/deps/mysql/include/) The docs (mysql.intro.requirements) gave me the impression that nothing should have been required beyond: TARGET_LINK_LIBRARIES(reviewtest Boost::mysql) # Are you knowledgeable about the problem domain? I have experience with MySql, Asio and related libraries. As alluded to before I've stopped short of creating an Asio-based async interface for MySql some years ago, when we were migrating a distributed server application to Asio (making the entirity of RPC paths non-blocking coroutines except for the database operations). We stopped short lack of time of time and experience. We've assessed Amy (https://github.com/liancheng/amy) around that time - which didn't seem ready for primetime. Instead I've invested time using mysql client lib as well as MySQL++ wrapper to get ORM-mapping to domain types from Mysql resultsets in that application. If anything, I respect that this library doesn't try to provide these functionalities (do one thing and do it well). I have experience with using a variety of other databases (mainly Oracle, MSSQL) in different tech stacks (.NET), which also adds some perspective. # Do you think the library should be accepted as a Boost library? I vote for acceptance. I think the library has a good foundation, focus and implementation quality. There are areas for improvements. Some of these will involve breaking interface changes, which would be best addressed before acceptance. Notes from review process: 1. The docs/examples need to show common usage patterns like INSERT with auto-increment fields. E.g. nothing the use of `last_insert_id()`. 2. The relational operations on mysql::value have technical meaning only. While the docs call this out, it forms a major pitfall. struct MyEntity { unsigned id; } entity{42}; try { conn.query("CREATE TABLE MyEntity AS SELECT 42 as id"); } catch (...) { } auto actual = conn.query("SELECT id from MyEntity").read_all().front().values().front(); M::value expected(entity.id); std::cout << actual << " == " << expected << "? " << std::boolalpha << (actual == expected) << "\n"; I'd say we need to remove the relational operations whole-sale OR implement them with Mysql semantics SELECT CAST(42 AS unsigned) = 42; -- MySQL: TRUE If the comparisons are required for internal purposes, they can be provided with custom comparator function objects. 3. The documentation claim "Values do not define `std::hash` yet, as `boost::string_view` does not support it" is out of date. `hash_value` has been added in 1.69.0 4. On that note, consider `boost::core::string_view` for better interop ("lingua franca" string_view). If you're interested I just did an exhaustive side-by-side comparison of features and observable behaviour` which I can share. 5. Others have mentioned multi-line queries. I'm wondering whether they add much seeing that sessions are already stateful: conn.query("set @greeting = 0x68656c6c6f20776f726c64;"); std::cout << "retained: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n'; std::cout << "retained: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n'; conn.query("set @greeting = 0x62796520776f726c64;"); std::cout << "modified: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n'; prints retained: hello world retained: hello world modified: bye world In fact it could be enough when `channel<>` does any required queuing. Unless queries are tiny and fragmented, there may not be a significant benefit. In fact, coalescing queries into a valid multi-line text form leads to allocation/copying overhead - and potential new safety risks. 6. The /one/ area where multi-line can shine is when sourcing hard-coded scripts, which begs the question: should a `connection::source(path/sqlscript)` feature be added instead? 7. "Finally, note that `prepared_statement`'s destructor does not perform any server-side deallocation of the statement. This is because closing a statement involves a network operation that may block your code or fail" This tells me that probably connections should contain a list of owned statement implementations that `prepared_statements` are a handle to. This is important when connection pooling, so prepared statements can be cleared before returning a connection to the pool. Otherwise this is simply a resource leak, and that's a problem. (Though some applications might prefer to give each connection in the pool a fixed set of statements. This doesn't suit all applications - heterogenous connection pools) 8. [mysql.other_streams.connection] If Windows named pipes is a common transport, I'd say providing first class connect/quit support is in order 9. Working with the Stream-parameterized type hierarchy is harder than necessary. E.g. `connection` has no inner typedef for `prepared_statement`/`resultset` or even the `Stream` template argument. This leaves one with work-arounds like: template <typename Conn> void my_function(Conn& conn) { using Stream = typename std::remove_reference<decltype(conn.next_layer())>::type; using Stmt = M::prepared_statement<Stream>; In general interface types (arguments/return values of member functions) should be directly expressible in code, if not through nested typenames, then at least through traits. 10. I did notice that in the integration tests there is a complete set of type-erased wrappers for resultset, connection, prepared_statement etc. Perhaps it is worth considering exposing this for users who are looking for convenience over raw speed. It could of course be that the type-erased wrappers have limitations (beyond efficiency) I'm not aware of. 11. A loose observation: `initializer_list<mysql::value>` is not practical due to explicit constructors. Of course, `make_values` facilitates that. (I'd keep the constructors explicit due to the fact that `mysql::value` has some surprising properties that make it bad idea to use as incidental type (ref strings and non-obvious relational ops, to name two)) 12. Seeing that Boost Mysql is header-only, what happens with the text segments (e.g. the error descriptions)? Is it worth providing alternative linkage options - conditional header-only compilation? Note: I didn't measure anything here. It was just a thought that crossed my mind when I compared to the stock mysql client libs. (On the plus side, there's no per-thread `mysql_library_init` call anymore, yay!) 13. tools/valgrind_suppressions.txt looks promising; I don't see where it is used. Is it up-to-date? 14. I'm not going to repeat some of the observations made by others, re. specific interfaces. I'm confident that these are going to be addressed. I **do** want to provide counter-point for the recurring theme that `mysql::value`'s string element type should be owning. I disagree. To me the primary purpose of the `value` type is to be a lightweight view that has utility efficiently marshaling to/from the wire format. To me it is more important to keep the 99% of use patterns efficient. We can instead clearly document that `mysql::value` is not a vocabulary type (and why) and not well suited for use outside the context of IO operations. Thanks Rubén for putting your considerable skill and time into this, and for being very responsive. Seth Heeren On Sun, May 15, 2022, at 6:36 PM, Ruben Perez via Boost wrote:

Thanks to all of you who have found the time to write a review to this point. I hope some more will be able to share your thoughts with us. The process has already provided me with a lot of useful information.

I've done my best to answer all your messages and questions, but the threads are sometimes difficult to follow. So if I've missed any of your messages, please let me know, as it hasn't been intentional.

Many thanks, Ruben.

On Sat, 14 May 2022 at 10:58, Richard Hodges via Boost <boost@lists.boost.org> wrote:

...

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

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

Rubén has also implemented the Asio protocols for deducing default completion token types :- https://anarthal.github.io/mysql/mysql/examples/default_completion_tokens.ht...

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@gmail.com office: +44 2032 898 513 home: +376 861 195 mobile: +376 380 212

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

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

Hi Seth, Thank you for taking your time to write a review. On Wed, 18 May 2022 at 04:43, Seth via Boost <boost@lists.boost.org> wrote:

My review of Boost Mysql

I'll loosely go by the guidelines [here](https://www.boost.org/community/reviews.html#Comments)

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

Highly useful.

In fact, my feeling was mostly "where was this library back in 2016". Back then we had to postpone necessary refactoring with big scalability benefits because I wasn't ready to do the async initiation function myself, or figure out how the mysql native client wants to do asynchrony.

Glad to hear.

# What is your evaluation of the design?

It is largely what I'd expect.

I might consider dropping the `*read_many` interfaces which has been been subject of discussion (mainly for returning `vector<row>`). I guess they could also be trivially improved by taking a reference to any mutable range of `row` objects instead (obviating the need to specify a count).

In my use, I find a "cursor-style" approach to consuming results (reusing the row buffer) the most natural way to async consume result-sets. Hence `read_one()` and perhaps `read_all()` would be the only interfaces I like to see.

They will likely disappear or be changed somehow to allow memory re-use. Tracked by https://github.com/anarthal/mysql/issues/58.

I'll probably have some more notes below.

# What is your evaluation of the implementation?

Pleasantly surprised.

A good example is when I zoomed in on the "generic execute" algorithm implementation. It looks completely readable, which is quite an accomplishment considering the domain.

Little cues picked up elsewhere (e.g. in tests) confirmed the impression.

This will positively affect both technical the maintainability and chances of community interest to support that.

Also, I think given a clean state of the implementation, the library could serve as a inspiration for people wishing to implement other protocols.

Thanks. Glad to hear.

# What is your evaluation of the documentation?

The documentation gave me a good idea of what the library does and doesn't do.

I'm not convinced of the "user-friendliness" angle. That's fine for me. It seems obvious that a library that implements a third-party wire-protocol from scratch is not focusing on high-level abstraction. It is good to manage the expectations in the documentation, though.

It's something that has definitely caused confusion during the reviews, so it may be worth rephrasing the docs to clearly state that this is a protocol library which can't be as user-friendly as a higher level one. https://github.com/anarthal/mysql/issues/70 tracks it.

I used GCC 10 on linux with CMake. I found I had to add the include path, which doesn't seem mentioned in the documentation:

INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/deps/mysql/include/)

The docs (mysql.intro.requirements) gave me the impression that nothing should have been required beyond:

TARGET_LINK_LIBRARIES(reviewtest Boost::mysql)

This shouldn't be the case at all. The second line should be sufficient. Could you please open an issue against the repo and post your CMakeLists.txt, please?

# Do you think the library should be accepted as a Boost library?

I vote for acceptance. I think the library has a good foundation, focus and implementation quality.

There are areas for improvements. Some of these will involve breaking interface changes, which would be best addressed before acceptance.

Thanks. I will address all these potentially breaking changes before getting the library into Boost (could take a while but necessary).

Notes from review process:

1. The docs/examples need to show common usage patterns like INSERT with auto-increment fields. E.g. nothing the use of `last_insert_id()`.

Seems fair. I've raised https://github.com/anarthal/mysql/issues/87 to track it.

2. The relational operations on mysql::value have technical meaning only. While the docs call this out, it forms a major pitfall.

struct MyEntity { unsigned id; } entity{42};

try { conn.query("CREATE TABLE MyEntity AS SELECT 42 as id"); } catch (...) { } auto actual = conn.query("SELECT id from MyEntity").read_all().front().values().front();

M::value expected(entity.id); std::cout << actual << " == " << expected << "? " << std::boolalpha << (actual == expected) << "\n";

I'd say we need to remove the relational operations whole-sale OR implement them with Mysql semantics

SELECT CAST(42 AS unsigned) = 42; -- MySQL: TRUE

If the comparisons are required for internal purposes, they can be provided with custom comparator function objects.

The only reason they are there is to allow values to be used in std::set, std::unordered_set and the like. As a user, I'd also find useful operator==, at least, to know "is the type and value of these two values the same"? The implementation doesn't rely on these operators. Tests do, but can be changed easily. Implementing MySQL semantics implies the following: value("1") == value(1) // true value(1.0) == value(1) // true Which reminds me of the kind of patterns that have brought us pain in PHP and JS, and would prefer to avoid. Do you think "placing values in containers" is something that adds value?

3. The documentation claim "Values do not define `std::hash` yet, as `boost::string_view` does not support it" is out of date. `hash_value` has been added in 1.69.0

I can see a definition of hash_value, and from what I've read this makes it compatible with Boost containers, but the std::hash specializations are in a #ifdef 0 block. AFAIR std::hash doesn't use hash_value, and std::unordered_set<boost::string_view> fails to compile. The same applies for std::unordered_set<boost::mysql::value>.

4. On that note, consider `boost::core::string_view` for better interop ("lingua franca" string_view). If you're interested I just did an exhaustive side-by-side comparison of features and observable behaviour` which I can share.

I wasn't aware of this class. Doing a quick grep in Beast, I've found it does use it, but it seems located in a detail/ folder (<boost/core/detail/string_view.hpp>). Could you please provide me with your comparison and any extra info I should be aware of? (BTW std::unordered_set<boost::core::string_view> doesn't compile either).

5. Others have mentioned multi-line queries. I'm wondering whether they add much seeing that sessions are already stateful:

conn.query("set @greeting = 0x68656c6c6f20776f726c64;");

std::cout << "retained: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n'; std::cout << "retained: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n';

conn.query("set @greeting = 0x62796520776f726c64;"); std::cout << "modified: " << conn.query("SELECT @greeting;").read_all()[0].values()[0] << '\n';

prints

retained: hello world retained: hello world modified: bye world

In fact it could be enough when `channel<>` does any required queuing.

Unless queries are tiny and fragmented, there may not be a significant benefit. In fact, coalescing queries into a valid multi-line text form leads to allocation/copying overhead - and potential new safety risks.

Not following you here. What do you mean by channel<> doing any queueing? It does have an internal buffer to re-use memory usage, but it doesn't implement any sort of queuing. I'd say that the pipeline interface (https://github.com/anarthal/mysql/issues/76) may be a better interface than multi-statement for this use case.

6. The /one/ area where multi-line can shine is when sourcing hard-coded scripts, which begs the question: should a `connection::source(path/sqlscript)` feature be added instead?

Agree. Raised https://github.com/anarthal/mysql/issues/88 to track it.

7. "Finally, note that `prepared_statement`'s destructor does not perform any server-side deallocation of the statement. This is because closing a statement involves a network operation that may block your code or fail"

This tells me that probably connections should contain a list of owned statement implementations that `prepared_statements` are a handle to.

This is important when connection pooling, so prepared statements can be cleared before returning a connection to the pool. Otherwise this is simply a resource leak, and that's a problem.

(Though some applications might prefer to give each connection in the pool a fixed set of statements. This doesn't suit all applications - heterogenous connection pools)

I'd say this is a problem specific to connection pooling and that should be implemented together with it (i.e. with https://github.com/anarthal/mysql/issues/19). When connections are closed, statements are de-allocated automatically, even if you didn't call prepared_statement::close explicitly (i.e. statements are local to the session established by the connection). So this only becomes a problem if you use the connection, prepare and execute some statements within that session, and then return the connection to the pool. I'd say to implement this as part of that issue. Note that prepared_statement's get allocated a server-side ID, which we expose via the prepared_statement::id() function. Something that could be useful for that use case is closing a statement without a prepared_statement object, just using its ID. I've noted all this in https://github.com/anarthal/mysql/issues/19 to prevent information from being lost.

8. [mysql.other_streams.connection] If Windows named pipes is a common transport, I'd say providing first class connect/quit support is in order

It is a supported transport - not aware of how many clients would be using it. There is two sides here: 1) Providing the relevant typedefs, docs and testing. This should be relatively straightforward. 2) Supporting the connection::connect and connection::close functions for named pipes. This involves opening a named pipe, which doesn't seem to have an async mode. Other than that, it should be doable.

9. Working with the Stream-parameterized type hierarchy is harder than necessary.

E.g. `connection` has no inner typedef for `prepared_statement`/`resultset` or even the `Stream` template argument. This leaves one with work-arounds like:

template <typename Conn> void my_function(Conn& conn) { using Stream = typename std::remove_reference<decltype(conn.next_layer())>::type; using Stmt = M::prepared_statement<Stream>;

In general interface types (arguments/return values of member functions) should be directly expressible in code, if not through nested typenames, then at least through traits.

Added the one for Stream as next_layer_type in develop. The other ones addressed in https://github.com/anarthal/mysql/issues/73 (see below).

10. I did notice that in the integration tests there is a complete set of type-erased wrappers for resultset, connection, prepared_statement etc. Perhaps it is worth considering exposing this for users who are looking for convenience over raw speed.

It could of course be that the type-erased wrappers have limitations (beyond efficiency) I'm not aware of.

They are more limited than the regular interface, as they only account for the use cases covered in the tests covering all the stream types. It should be possible to tidy them up and ship them to the general public, though. I hadn't really thought on this before. Something I noticed while implementing these erased types is that having prepared_statement and resultset be I/O objects makes the erased interface much harder. As I've mentioned in other review responses, I'm thinking on replacing the following signatures: prepared_statement::execute(const statement_params<ValueIterator>&) prepared_statement::close() resultset::read_one(row&) By functions in the connection: connection::execute_statement(const prepared_statement&) connection::close_statement(const prepared_statement&) connection::read_row(resultset&, row&) Where prepared_statement and resultset would be non-templated types. What do you think of this API?

11. A loose observation: `initializer_list<mysql::value>` is not practical due to explicit constructors.

Of course, `make_values` facilitates that.

(I'd keep the constructors explicit due to the fact that `mysql::value` has some surprising properties that make it bad idea to use as incidental type (ref strings and non-obvious relational ops, to name two))

12. Seeing that Boost Mysql is header-only, what happens with the text segments (e.g. the error descriptions)? Is it worth providing alternative linkage options - conditional header-only compilation?

Note: I didn't measure anything here. It was just a thought that crossed my mind when I compared to the stock mysql client libs. (On the plus side, there's no per-thread `mysql_library_init` call anymore, yay!)

It's something on the roadmap - that's why the implementation is split in .ipp files. https://github.com/anarthal/mysql/issues/23 tracks it.

13. tools/valgrind_suppressions.txt looks promising; I don't see where it is used. Is it up-to-date?

If you look into cmake/test_utils.cmake, there is a function add_memcheck_test that uses that file. All examples excepting the Boost.Coroutine one, as well as a subset of integration tests, are memchecked. This is run in one CI job.

14. I'm not going to repeat some of the observations made by others, re. specific interfaces. I'm confident that these are going to be addressed.

I **do** want to provide counter-point for the recurring theme that `mysql::value`'s string element type should be owning. I disagree. To me the primary purpose of the `value` type is to be a lightweight view that has utility efficiently marshaling to/from the wire format.

To me it is more important to keep the 99% of use patterns efficient.

We can instead clearly document that `mysql::value` is not a vocabulary type (and why) and not well suited for use outside the context of IO operations.

Vinnie also made your same point, suggesting having an owning_value as counterpart. The row type would expose the lightweight view, and the user can then construct the owning_value from it. https://github.com/anarthal/mysql/issues/85 tracks it.

Thanks Rubén for putting your considerable skill and time into this, and for being very responsive.

Thanks.

On Thu, 19 May 2022 at 20:47, Julien Blanc via Boost <boost@lists.boost.org> wrote:

My (small) review of Boost MySQL.

Hi Julien, thanks for taking your time to write your review.

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

Glad to hear.

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

Could you please open an issue against the repository pointing out where you found these?

* 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 MySQL to C++ type reference briefly talks about this (https://anarthal.github.io/mysql/mysql/types.html). The string_view contents will be the raw bytes in all cases. In your example, the first string_view will be utf8 encoded, the second utf16 encoded, and the third one will be in iso8859-15. The WHERE clause is part of the SQL statement you're issuing, so it should be encoded using the connection's encoding (as given by connection_params::collation). I agree the documentation on this topic is scarce, and it's a common source of confusion for users. I've raised https://github.com/anarthal/mysql/issues/94 to tackle this.

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

This is explained pretty thoroughly here: https://anarthal.github.io/mysql/mysql/types.html. MySQL DATETIME fields represent "naive" datetime objects (similar to Python's datetimes with tzinfo=None). It's up to the user to interpret which time zone they are in. MySQL TIMESTAMP fields also represent datetime objects. However, these are timezone aware, but the timezone is not stored with the value. When you insert a value into a TIMESTAMP column, MySQL converts the timestamp into UTC and stores it as UTC. When you query the field, by default, it will convert the timestamp back to your local timezone. "Your timezone", in this context, means the timezone given by the session-local time_zone variable (https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_...). The time_point object you get, in this case, is a local time (and NOT UTC), and the time zone is given by the time_zone variable. I haven't tried to be smart and convert to UTC because the time_zone variable can change programmatically, by issuing a `SET` SQL command, so its contents can't be tracked reliably. Full MySQL docs here: https://dev.mysql.com/doc/refman/8.0/en/datetime.html. It may be worth adding an example on how to work with these types. https://github.com/anarthal/mysql/issues/95 tracks it.

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

These were chosen because of conversions. So the following works: value v (uint64_t(200)); auto opt = v.get<int64_t>(); // This will perform a conversion because 200 fits in a int64_t I need to reconsider this because many of you have raised it. A get_if interface means that these conversions must go away (or be reduced to just value::get).

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

Thanks for offering a possible solution to this problem, as many have flagged it already. I've updated https://github.com/anarthal/mysql/issues/85 with your proposal.

* 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

This will be addressed as part of https://github.com/anarthal/mysql/issues/58, which will be tackled before inclusion into Boost. I like your idea, and other users have proposed that I use custom ranges here, too, which I will likely do. I'm not convinced of the row visiting mechanism - I would say this is mixing concerns. I would say the rows should offer a mechanism to access the values (e.g. by index or iteration), and then the values should be visitable.

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

std::chrono::days doesn't specify which integer type is used to represent the duration, it only states that it is signed. I used plain int, which fits the purpose, but may be different to std's. The only possibility here would be to detect if std::chrono::days is available and define boost::mysql::days differently depending on whether it is or it is not. But this can then cause ODR violation issues if a user uses the library under two different C++ standard levels (possibly without even knowing it, because of a dependency). That's why this is defined like this.

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

Thanks. I will try to address as many of these changes before it is available to the general public.

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

Thank you for your time.

Regards,

Julien

Regards, Ruben.