Re: MySql review

From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2022-05-13 15:23:55

Ruben Perez wrote:

>> I note that "Ease of use" is claimed as the first design goal,
>> which is good.
>
> I think I failed to make the scope of the library clear enough in this
> aspect. The library is supposed to be pretty low level and close to the
> protocol, and not an ORM.

It seems to me that what you have *could* be a genuinely easy-to-use
library with a relatively small amount of extra work. Specifically,
if you were to hide the initial ASIO boilerplate then synchronous
queries could be quite concise and require no ASIO knowledge.

By chance last night I noticed this linked from the isocpp.org website:

    Writing a Network Client with Facebook's Wangle is a Fail
    by Richard Thomson
    From the video description:

    Networking is often a core requirement for modern applications, but
    the standard C++ library doesn't yet include any networking support.
    The Boost libraries have had networking components in the ASIO
    library, but it looks complicated and filled with details.
    [snip]

I certainly agree with "looks complicated"; I have never summoned the
energy to dive into it. So why would you want to limit the audience
of your library only to those who are already ASIO users or are
willing to invest in learning to use it, when with the addition of
just a few more convenience functions you could hide ASIO as an
implementation detail for a useful subset of your functionality?

> If you take a look to any other asio-based program, the user is
> always in charge of creating the io_context, and usually in charge of
> creating the SSL context, too.

But take a look at any other database connection library and you
won't find that.

> I'm not keen on creating a function that both resolves the hostname
> and connects the connection, as I think it encourages doing more
> name resolution than really required (you usually have one server
> but multiple connections).

It's a mistake to keep an IP address for too long; you should re-resolve
the hostname based on the time-to-live. Consider the case where the
server has failover DNS or load balancing. Does the ASIO resolver
cache based on the TTL? If it does, those calls should be cheap.

>>   make_connection("mysql://admin:12345_at_hostname:3306/dbname");
>
> I guess you're suggesting that make_connection also perfor 
> the name resolution, the physical connect and the MySQL handshake?

Yes, that's exactly what I'm suggesting. See e.g. PQconnectdb()
in libpq.

>> Another point about the connection parameters: you should
>> provide a way to supply credentials without embedding them
>> in the source code. You should aim to make the secure option
>> the default and the simplest to use. I suggest that you
>> support the ~/.my.cnf and /etc/my.cnf files and read passwords
>> etc. from there, by default. You might also support getting 
>> credentials from environment variables or by parsing the
>> command line. You could even have it prompt for a password.
>
> I don't know of any database access library that does this.

In libpq, I normally pass "service=foo" as the connection parameter
string and it looks that up in pg_service.conf. I think that can
specify a password. It also supports a ~/.pgpass file.

The AWS SDK for DynamoDB looks for credentials in a ~/.something
file.

The aim should be "secure by default". Users are lazy. The
particular danger in this case is that they do an initial test with
the password in the source, and then move it somewhere secure later,
but the password is still exposed in their revision control history.
At this point in history, there is no excuse to repeat the mistakes
that have lead to really very serious security problems in the past.
Make the default mechanism, and the first one that you describe in
the docs, the most secure one.

> Just use std::getenv, std::stdin or whatever mechanism your application
> needs and get a string from there, then pass it to the library.
> All the examples read the password from argv.
> Additionally, having passwords in plain text files like
> ~/.my.cnf and /etc/my.cnf is considered bad practice in terms
> of security, I wouldn't encourage it.

The command line and the environment are potentially just as exposed
as files, via /proc. If you want, refuse to read from a ~/. file
if it is world-readable. lpbpq does this for .pgpass.

>> Invoking the prepared statement seems unnecessarily verbose.
>> Why can't I just write
>>
>>   auto result = my_query("hello", "world", 42);
>
> Because this invokes a network operation. By Asio convention,
> you need a pair of sync functions (error codes and exceptions)
> and at least an async function, that is named the same as the sync
> function but with the "_async" suffix.

Make operator() an alias for execute().

It would be good if it could also be used in the coroutine case,
i.e. if my connection_type is based on a coro_executor_type then
maybe operator() could be an alias for async_execute:

     auto r = co_await query.async_execute(params);
or  auto r = co_await query(params);

>>   auto result = ...execute query...
>>   for (auto&& row: result) {
>>     ...
>>   }
>
> How does this translate to the async world?

>> I have found it useful to have a Transaction class:
>>
>>   {
>>     Transaction t(conn);  // Issues "BEGIN"
>>     .... run queries ....
>>     t.commit();           // Issues "COMMIT"
>>   }                       // t's dtor issues "ROLLBACK" if we have not committed.
>>
>
> Again, how would this work in the async world?

I'm far from an expert, but you should be able to write code something like:

   auto result = co_await query(params);
   for co_await (auto&& row: result) {
     ...
   }

   auto t = co_await Transaction(conn);
   ....
   co_await t.commit();
   // No idea about how to await the destructor, sorry.

Of course it's much more horrible if you're using callbacks, but then
everything is horrible with all those nested lambdas. Heck, it's difficult
even to write a loop!

> How does the destructor handle communication failures
> when issuing the ROLLBACK?

I suggest mark the connection as broken but otherwise ignore the error.

>> I use NUMERIC quite a lot in PostgreSQL; I don't know if the
>> MySql type is similar. I would find it inconvenient that it
>> is treated as a string. Can it not be converted to a numeric
>> type, if that is what the user wants?
>
> MySQL treats NUMERIC and DECIMAL the same, as exact
> numeric types. What C++ type would you put this into?
> float and double are not exact so they're not fit.

Provide a mechanism that allows the users to indicate what
type they want to convert them to.

Regards, Phil.