Added a generic connection pool

[iriselia]

Added a very basic outline of a generic connection pool. It should be
thread-safe, but still needs a lot of work.

[rbock]

Namespace mysql is probably a copy&paste error, right?

[iriselia]

Oops, good catch... Should I submit another pull request?

[rbock]

You can also update it somehow (github will guide you through that).

[iriselia]

fixed namespace error.

[rbock]

If you're using shared_ptr, the question is: Who owns the connection?

Alternatively, you could maintain a stack of unique_ptr. When someone requests a connection, you take that it from the stack and hand it out (or you create a new one if the stack is empty). When it is being returned, you check the size of the stack and either put the connection back or discard it.

[iriselia]

I see. I agree that using a stack of unique_ptr is much safer. I used shared_ptr here because I was trying to avoid spending cpu cycles on pushing and popping connections. I'll go ahead and edit as requested.

[iriselia]

There's an issue with the stack approach and that is I cannot check whether or not the connection being handed back is originally from the same connection pool. How should we address this issue?

[rbock]

I'd say that's rather unlikely to happen.

It should be documented that returned transaction need to come from the same pool or at least have the same configuration.

[rbock]

Please also add a copyright notice. FreeBSD 2-clause like the rest of sqlpp11 would be preferred.

[iriselia]

Will do! I'll submit a new version soon. Frank

[iriselia]

Copyright notice added. Documentation on free_connection added.

[Erroneous1]

You might want to add yourself to the copyright and update for 2017.

Might need

#include <iostream>
#include <sqlpp11/exception.h>

If the user wanted to see why a connection couldn't be created, they couldn't see why without monitoring std::cerr. I think throwing might be more consistent with the library. Maybe change to:

} catch(const sqlpp::exception& e) {
    std::cerr << "Failed to spawn new connection." << std::endl;
    throw;
}

maximum_pool_size might be more descriptive than default_pool_size.

If you wanted make a connection pool that didn't allow mixing connection configs, you could have something like this:

template<class Connection>
sqlpp::is_same_config(const std::unique_ptr<Connection>& con, const std::shared_ptr<typename Connection::connection_config>& cfg) {
    return con->config.get() == cfg.get();
}

Then the connectors could add some typedefs or overload this as needed. I know the ODBC connector would also need to add a contructor for const std::shared_ptr<connection_config>& and should switch to keeping a shared_ptr instead of a full connection_config object.

[iriselia]

Thanks for the suggestions, I just submitted the changes.

Initially I was thinking of having a maximum_pool_size to limit the number of temporary connections, but I also realized it might not be necessary, and removed it in a later version. Since the current behavior of the connection pool is that if the user requests more connections than default_pool_size, create new temporary connections. If we renamed default_pool_size to maximum_pool_size, I worry the user would assume they will have to resize the pool manually. Am I overthinking it?

I added the sqlpp::is_same_config as you suggested and also updated the mysql connector to support this interface. #24

[rbock]

maximum_pool_size sounds about right to me. The pool size never growths beyond that limit, right?

If the pool is empty, it will create new connections, if "too many" connections are returned, it will drop the ones beyond the maximum.

I would start with an empty pool, btw., and just follow the rule above.

It might be necessary to test if connections are still "alive". I guess it would be possible to get disconnected from the server.

[iriselia]

I think you are right, I can finally wrap my head around the maximum_pool_size now.
Starting with an empty pool sounds good to me. I've pushed the change for that.

I think if we want to check the status of the connection we might need to expand the connectors so that we can do conn->is_valid() and conn->reconnect().

[rbock]

I think if we want to check the status of the connection we might need to expand the connectors so that we can do |conn->is_valid()| and |conn->reconnect()|.

Agreed.

[iriselia]

I looked at a stack overflow answer on connection keep-alive policy: http://stackoverflow.com/questions/2546868/cheapest-way-to-to-determine-if-a-mysql-connection-is-still-alive

It seems like doing a mysql_ping() prior to giving out a connection from the pool is not the most ideal approach. Should we still enforce such a policy or should we implement some sort of keep-alive policy and make the necessary assumptions?

[rbock]

Hmm. Probably a matter of preferences. You could add a template parameter that determines whether or not a ping() or is_valid should be executed.

Another thought about obtaining and returning connections: This is a classic RAII candidate. You could wrap the connection into an object that is responsible for returning the connection to the pool upon destruction.

Obviously, the pool has to outlive the the connection for this to work, but that would be the normal case anyway, I guess.

[iriselia]

I added an async_connection wrapper to utilize RAII. I am not very familiar with the difference between is_valid() and mysql_ping() in the oracle mysql c++ connector. Right now I have is_valid() set to return !!_handle; to make sure we are not moving an empty connection back to the pool. Of course, this is just for implementing the conditional RAII, not for checking whether or not we have disconnect from the DB server.

[rbock]

Hi Frank,

I mentioned ping and is_valid just because I guess different vendors will offer different functions.

Why do we need the async_connection to be wrapped in a unique_ptr? I would have done something like

class pool_connection // not using async_connection, as the pool is probably useful in non-async contexts as well
{
  unique_ptr<connection> _impl;
  connection_pool* origin;
  
public:
  template<typename... Args>
  auto operator()(Args&&... args) -> decltype(_impl->args(std::forward<Args>(args)...))
  {
     return _impl->args(std::forward<Args>(args)...);
  }
  // Rest of interface as required.
};

A bit more wrapper code, but easier to use, for instance with operator(), IMO.

Cheers,

Roland

[iriselia]

Hi Roland,
I could certainly change it to your approach. I started with the inheritance approach because I wasn't sure if it's ok to simply return a plain pool_connection when calling get_connection(). Since you basically just said I don't have to, I definitely prefer your approach over _impl. Without a unique ptr the operator() would also become tremendously useful. I'll submit the relevant changes soon.

[rbock]

Cool.

I will have to slow down a bit. I am giving a talk about the ongoing port to C++17 on Thursday. That will use up most of my spare time til then.

Best,

Roland

[iriselia]

Not a problem. Good luck with the talk! I'll look at other stuff I can work on until then.

Frank

[Erroneous1]

I commited some prep work on Erroneous1/sqlpp11-connector-odbc@1226070 that should make it compatible for this PR. You can try it out if you'd like to have a connector for testing.

So that nothing can change the config data I'm using const std::shared_ptr<const connection_config>. I believe this would just change the typename Connection_config so it shouldn't be an issue. This also allows for the auto_reconnect configuration parameter which seems to work in my tests. If you don't have auto_reconnect turned on it tests the connection by getting a table list.

I also did some testing on my is_valid() function. On my setup it takes around 1ms to run with auto_reconnect turned off. When the connection is invalid it only takes about 0.2ms.

[iriselia]

Awesome, high quality code on the first commit! I wish I were as competent as you :)

I think putting auto_reconnect in the config is a great idea. I think it makes perfect sense when auto_reconnect is set to true to let the connection_pool handle all the errands before handing out a reliable connection. However I am unsure how it should affect the behavior of our connection_pool when we have config.auto_reconnect = false;, we should still somehow make sure the async_connection we are handing out is usable, right? Or do we have it so the user needs to validate the connection manually as needed? Regardless I think it's never desirable to give out an unreliable connection.

Then again I really hope in a production environment where db servers are not always local do not suffer from constant connection checking with is_valid();

Frank

[iriselia]

Updated connection_pool again. Cleaned up the implementation, and changed the async_connection from derived class of Connection type to a container of Connection type.

[Erroneous1]

I think reconnecting as necessary when handing out connections is fine. There are 2 times I can think of when a user wouldn't want you to check is_valid() and reconnect. 1 is when is if is_valid() is costly and the user only wants it do only happen every so often. 2 is if reconnecting might (and does with ODBC) invalidate any prepared statement or transactions. The 2nd isn't a huge thing because they'd most likely be invalidated if they got a different connection handle anyway. If you use async you should finish any transaction or prepared statement before getting a new connection.

What if we added to the template for connection pool a validator class? You'd call operator() when handing out a connection from the pool and call clean() when that pointer is about to be invalidated (see connection_validate_after).

struct connection_validator {
    template<typename Connection>
    void operator()(Connection* con) {
        if(!con->is_valid())
            con->reconnect()
    }
    template<typename Connection>
    void clean(Connection* con) {}
};

class  connection_validate_after {
private:
    std::chrono::seconds revalidate_after;
    std::map<void*,std::chrono::time_point<std::chrono::system_clock> > last_checked;
    std::mutex last_check_mutex;

public:
    connection_validate_after(const std::chrono::seconds r=28800) //default wait_timeout in MySQL
        : revalidate_after(r), last_checked(), last_check_mutex()
        {}

    template<typename Connection>
    void operator()(Connection* con) {
        std::lock_guard<std::mutex> lock(last_check_mutex);
        auto last = last_checked.find(con);
        auto now = std::chrono::system_clock::now();
        if(last == last_checked.end()) {
            if(!con->is_valid())
                con->reconnect();
            last_checked.emplace_hint(last, con, now);
        } else if(now - last->second > revalidate_after) {
            if(!con->is_valid())
                con->reconnect();
            last = now;
        }
    }
    template<typename Connection>
    void clean(Connection* con) {
        std::lock_guard<std::mutex> lock(last_check_mutex);
        auto itr = last_checked.find(con);
        if(itr != last_checked.end())
            last_checked.erase(itr);
    }
};

struct connection_never_validate {
    template<typename Connection>
    void operator()(Connection*) {}
    template<typename Connection>
    void clean()(Connection*) {}
};

[iriselia]

My apologies for the late reply, I was caught up with school for a few days. I like your approach very much. I am going to add them asap. I have also started working on asio async query and callback. I should be able to submit a draft version soon.

Edit: I quickly realized if we let validator types be a new template type, i.e.

template <typename Connection, typename Connection_config, typename Connection_validator>
class connection_pool
{
    etc...

it would be difficult to access the revalidate interval in the connection_validate_after and potentially other states that customized validators may have. If I were to implement them through inheritance and virtual functions, i.e.

template<typename Connection>
struct reconnect_policy
{
	virtual void operator()(Connection* connection) = 0;
	virtual void clean(Connection* connection) = 0;
};

template<typename Connection>
struct auto_reconnect : public reconnect_policy<Connection>
{
	template<typename Connection>
	void operator()(Connection* connection)
	{
		if(!connection->is_valid())
			connection->reconnect()
	}
	template<typename Connection>
	void clean(Connection* connection) {}
};

template <typename Connection, typename Connection_config>
class connection_pool
{
public:
    connection_pool(const std::shared_ptr<Connection_config>& config, unsigned int pool_size, connection_validator validator)
        : validaor(validator), config(config), maximum_pool_size(pool_size) {}

I am also afraid the use of virtual functions for such a small task would be too costly. What do you think we should do here?

Frank

[iriselia]

Also currently in order to create a connection_pool one needs to do something like this:

auto config = std::make_shared<sql::connection_config>();
config->user     = "root";
config->password = "password";
config->database = "sqlpp_mysql";
config->debug    = true;

sqlpp::connection_pool<sql::connection, sql::connection_config> pool(config, 4);

With this line sqlpp::connection_pool<sql::connection, sql::connection_config> pool(config, 4); in particular, I think having 2 template parameters is redundant. What if we stored the entire connection_config type within connection so we can access it like so: sql::connection::config? This way we can eliminate the second template parameter and just do sqlpp::connection_pool<sql::connection> pool(config, 4);

[Erroneous1]

We could make the connectors add typedef connection connection; to the connection_config and then do something like:

template<typename Config, typename Connection = typename Config::connnection, typename Reconnect=auto_reconnect>
connection_pool<Connection, Config, Reconnect> make_connection_pool(
        const std::shared_ptr<Config>& config,
        size_t max_pool_size,
        reconnect=Reconnect()
) {
    return connection_pool<Connection, Config, Reconnect>(config, max_pool_size, reconnect);
}

That should let you do something like auto pool = sqlpp::make_connection_pool(config, 4);. Only thing I don't like about that is the order. If the connector didn't implement config::connection it would still work with sqlpp::make_connection_pool<connection>. If you wanted to specify a validator though it would be a pain. sqlpp::make_connection_pool<connection, connection_never_validate>. Alternatively, the connectors could add their own typedefs:

namespace sqlpp {
    namespace odbc {
        template<typename Reconnect=auto_reconnect>
        using connection_pool = sqlpp::connection_pool<sqlpp::odbc::connection, sqlpp::odbc::config, Reconnect>;
    }
}

As for the reconnect_policy, we could remove the mutex stuff if we validate before closing the lock_guard for the connection_pool when getting a connection. No need for double mutex protection in a single operation now that I think about it. I think that using a base class and virtual functions would probably make it difficult to optimize out, so it would probably be best not to do that if possible. The ideal is for something like Howard Hinnant's date library where using all of our nice abstraction and compiling with high optimization produces the same ASM as doing it by hand.

[rbock]

Regarding the make_connection_pool I am sure you can make it nicer with a few overloads that distinguish between configs with or without connection type via enable_if.

Or you could just assume that whoever uses the connection pool, will have a config type that knows its connection type. Since the pool is a new feature that seems like a reasonable assumption to me.

[iriselia]

Okay! I'll see what I can do. Thanks.

Frank

[iriselia]

1. I extracted pool_connection to a separate file because it seems to comply better with the rest of the library. If it is unnecessary I will revert the related changes.

2. I also added the reconnect policies Erroneous1 provided. I put them in a separate namespace to avoid contaminating the sqlpp:: namespace

3. I also expanded the connection_pool class template and constructor with enable_if and default parameters to allow lazy instantiation. I also added the make_connection_pool helper function provided by Erroneous1 to eliminate angle brackets. The user can now do the folowing:

sqlpp::connection_pool<sqlpp::mysql::connection_config> pool1(config, 4);
sqlpp::connection_pool<sqlpp::mysql::connection_config, sqlpp::reconnect_policy::periodic_reconnect> pool2(config, 4);
// For connectors that are not up to date:
sqlpp::connection_pool<sqlpp::mysql::connection_config, sqlpp::reconnect_policy::auto_reconnect, sqlpp::mysql::connection> pool3(config, 4);

// or alternatively
auto pool4 = sqlpp::make_connection_pool(config, 4);
auto pool5 = sqlpp::make_connection_pool<sqlpp::mysql::connection_config, sqlpp::reconnect_policy::auto_reconnect>(config, 4);
// For connectors that are not up to date:
auto pool6 = sqlpp::make_connection_pool<sqlpp::mysql::connection_config, sqlpp::reconnect_policy::auto_reconnect, sqlpp::mysql::connection>(config, 4);

[rbock]

It would be awesome if you could provide some text for the wiki, too.

But you might also want to wait a bit until the async parts are available and we "know" that we want to stick with the current interface.

[iriselia]

Sure thing. I think the new files can still use some refactoring before going into the wiki. Also in case we come across new problems implementing async callbacks and need to change the design I would like to give it a bit more time to consolidate.

[csboling]

Super cool. I was playing with this to see if pool_connections could still be used to run dynamic queries, and I thought it would work to say

auto conn = pool.get_connection();
sqlpp::dynamic_update(*conn.operator->(), . . .);

but any invocation of pool_connection::operator-> gives me

sqlpp11/pool_connection.h(65): 'return': cannot convert from 'std::unique_ptr<Connection,std::default_delete<_Ty>> *' to 'DB::connection *' 
          with
          [
              Connection=DB::connection,
              _Ty=DB::connection
          ]

where DB::connection is my connection type. Should the implementation of pool_connection::operator-> read return _impl.get();? Is this the right way to get a connection object from a pool_connection to call dynamic query functions and the like that want a connection reference?

[iriselia]

I did not consider this when I wrote it, thanks for posting this. I'll take a look and see what can be improved.

Frank

[iriselia]

Issue created: #162.