Constantly keeping 10 idle connections about reactor-pool

[petromir]

And then I can safely call ConnectionPool pool = new ConnectionPool(configuration);

[simonbasle]

@petromir as discussed in the r2dbc-pool issue, without activity on the pool you need background eviction enabled for it to dispose of idle objects that don't pass the eviction predicate anymore. so this needs to be implemented in r2dbc-pool directly (which should be easy to contribute).

[petromir]

@simonbasle Thanks for your reply!
Unfortunately using background eviction doesn't help to close the actual connection, just making pool statistics look right.

The issue is easy to reproduce:

1. Clone my version of r2dbc-pool with added support for background eviction.
2. Run mvn clean install -DskipTests
3. Clone https://github.com/petromir/r2dbc-pool-idle-connection-issue and follow the steps defined in the repository.

As a result we have JMX statistics looking good (with 0 idle connection), BUT still 10 database connections in idle state.

select state, count(pid), query FROM pg_stat_activity where datname = 'books' and pid<>pg_backend_pid() and application_name = 'r2dbc-postgresql' group by state, query;

state	count	query
idle	10	INSERT INTO book (title, author) VALUES ($1, $2) RETURNING *

Am I missing something?

I've noticed that there is .releaseHandler(Connection::close) which results in two exceptions

• Caused by: java.lang.IllegalStateException: The connection is closed
  This seems to be r2dbc-pool specific as we have assertion whether io.r2dbc.pool.PooledConnection is closed in several places

private void assertNotClosed() {
        if (this.closed) {
            throw new IllegalStateException("The connection is closed");
        }
}

• Inability to acquire a connection.

java.util.concurrent.TimeoutException: Did not observe any item or terminal signal within 7000ms in 'Connection Acquisition from [ConnectionPool[PostgreSQL]]' (and no fallback has been configured)

[simonbasle]

I see that in your fork you've enabled background eviction but also switched from a releaseHandler to a destroyHandler for Connection::close. Can you confirm that if assertNotClosed exception is caught and turned into a Mono.error in that destroyHandler function, it improves the situation?

[simonbasle]

(there might indeed be an issue with destroyHandlers that throw from the Function instead of returning a Mono.error. reopening)

[petromir]

@simonbasle The only thing that I have done in my version is adding of .evictInBackground(maxIdleTime) during pool construction. See original version vs. mine
It will take a bit more time until I convert all the invocations of assertNotClosed to Mono<Void>.

[simonbasle]

@petromir Can you verify latest snapshots (from the merge of #139) with your r2dbc-pool fork and validate it fixes your reproduction case?

[petromir]

@simonbasle when 0.2.5 will be available in Maven Central?

[simonbasle]

@simonbasle when 0.2.5 will be available in Maven Central?

Next tuesday (updated the milestones). If you could verify the fix using snapshots today, that would be perfect.

[petromir]

I will do my best 😄

[simonbasle]

I had some time to investigate, and despite the changes I've made I was still seeing 10 idle connections... Then something felt off, so I tried playing around with the configuration of the pool in the app.

I've set the maximum size to 5 and wrapping the ConnectionFactory so that create() calls could be counted. And indeed, the application makes less than 5 calls to create()...

But still, pg tells me that there are 10 idle connections 🤔

And then I noticed that ConnectionPoolConfiguration#build from r2dbc-pool is called twice... and the first time it uses default values for minSize and maxSize, which happens to be 10 !

So it hit me: you're configuring a pool around a pool:

final ConnectionFactory connectionFactory = ConnectionFactories.get(ConnectionFactoryOptions.builder()
                .option(DRIVER, "pool")
                .option(PROTOCOL, "postgresql")

The logs even plainly warn you about it at the start of the application !! 🤦‍♂️

WARN 53028 --- [ main] io.r2dbc.pool.ConnectionPool : Creating ConnectionPool using another ConnectionPool [ConnectionPool[PostgreSQL]] as ConnectionFactory

Using the postgresql driver directly as basis for the r2dbc-pool, with background eviction, correctly makes these idle connections disappear from pg's perspective.

[petromir]

Didn't get what is wrong with the pool configuration as the example in r2dbc-pool was used as a source

ConnectionFactory connectionFactory = ConnectionFactories.get(ConnectionFactoryOptions.builder()
   .option(DRIVER, "pool")
   .option(PROTOCOL, "postgresql") // driver identifier, PROTOCOL is delegated as DRIVER by the pool.
   .option(HOST, "…")
   .option(PORT, "…") 
   .option(USER, "…")
   .option(PASSWORD, "…")
   .option(DATABASE, "…")
   .build());

[simonbasle]

@petromir you're mixing two methods of creating ConnectionPools: discovery with options and full programmatic builder-based creation.

This creates 3 ConnectionFactory:

1. discovery via ConnectionFactories.get(...).build() creates a ConnectionPool (DRIVER = pool), which is a ConnectionFactory
2. it also creates (and wraps over) a postgresql ConnectionFactory (PROTOCOL = postgresql)
3. new ConnectionPool(configuration) explicitly creates a ConnectionPool wrapping over (1) which itself wraps over (2).

(1) is created with default configuration (because no particular pool-related option(...) have been set other than the PROTOCOL). This amounts to minSize = 10 and no background eviction.

(3) is the outer pool, and is indeed configured with background eviction and minSize 0 this time.

As soon as a connection is requested from that second pool, it will hit the inner pool which will warm up 10 connections and attempts to keep them around (minSize = 10), despite the outer pool being configured for background eviction and minSize 0.

[petromir]

Well this seems to be quite confusing for users of r2dbc-pool as configuring the pool programmatically is exactly in the way I did it

ConnectionFactory connectionFactory = …;

ConnectionPoolConfiguration configuration = ConnectionPoolConfiguration.builder(connectionFactory)
   .maxIdleTime(Duration.ofMillis(1000))
   .maxSize(20)
   .build();

ConnectionPool pool = new ConnectionPool(configuration);

What should be the right way to combine both settings, so that we don't have several connection pools created?

[simonbasle]

I don't think the intention was for these ... to represent a ConnectionPool instantiation.
Rather, it should be the typical instantiation code for whichever plain driver you want/need underneath the pool, in that case postgresql.

Simplest way to fix your sample is to have the discovery instantiation directly discover postgresql connection factory:

final ConnectionFactory connectionFactory = ConnectionFactories.get(ConnectionFactoryOptions.builder()
                .option(DRIVER, "postgresql") //postgres is the DRIVER now, removing the need for PROTOCOL
//the rest stays as is
                .option(PORT, port)
                .option(HOST, "localhost")
                .option(USER, "root")
                .option(PASSWORD, "password")
                .option(DATABASE, "books")
                .option(Option.valueOf("preparedStatementCacheQueries"), 0)
                .build());

//here everything stays the same since now `connectionFactory` is not a pooled one
final ConnectionPoolConfiguration configuration = ConnectionPoolConfiguration.builder(connectionFactory)
                .initialSize(0)
                .maxSize(20)
                .maxIdleTime(Duration.ofMillis(2500))
                .maxCreateConnectionTime(Duration.ofSeconds(10))
                .acquireRetry(3)
                .maxAcquireTime(Duration.ofSeconds(7))
                .registerJmx(true)
                .name("r2dbc")
                .build();

[petromir]

@simonbasle I've tested with the snapshot locally and I can confirm that everything works as expected, but only with the DRIVER trick. Thanks a lot!