Reuse JDBC connection in JDBC connectors

[kokosing]

Reuse JDBC connection in JDBC connectors

[kokosing]

It does not work somehow. Things do not get committed.

[kokosing]

There is no need to make it functional. Just make the class not static.

[lhofhansl]

So these are cached in each worker for the duration of a query and after the query is done all connection(s) are released...?
Asking because holding connections open can tie up non-trivial resources - server-side - on some databases.

[kokosing]

So these are cached in each worker for the duration of a query and after the query is done all connection(s) are released...?
Asking because holding connections open can tie up non-trivial resources - server-side - on some databases.

This is in progress. I don't want to hold connections open for a long time. Now I also added a change to keep it up to 2 seconds when innactive.

[findepi]

good catch

Can you move it to a separate PR?

[kokosing]

#14702

[findepi]

i have some comments in #14702.
please apply & rebase

[kokosing]

improve commit message grammar

[kokosing]

inline kvCache

[findepi]

"Introduce NonLoadableCache"

[findepi]

i have some comments in #14702.
please apply & rebase

[findepi]

This should also test that these make the cache empty.

[kokosing]

I don't think so. SafeCaches provides forwarding implementation of cache, so it is up to a delegate cache to clear things. So testing if methods are properly implemented should be on a different level, where a delegate cache implementation is.

[findepi]

What's the rationale to have NonLoadableCache interface and NonLoadableCacheImpl as separate classes?
I.e. would anything bad happen if the implementation class was called NonLoadableCache, was public and had a package-private constructor?

[findepi]

The cache eviction problem isn't limited to get(K key, Callable<? extends V> loader) method.
Even if I call asMap().putIfAbsent etc i insert into a cache a value that's "fresh" from the calling thread perspective, but can already stale from some other thread perspective.
Thus this safety is an illusion, the NonLoadableCache isn't any more safe than any other Guava cache. It's just less convenient to use.

[findepi]

"Introduce QueryEventListener to JDBC connectors"

[findepi]

"Static import SINGLETON"

[findepi]

For some reason....

/trino master$ git grep '.in(SINGLETON)' | wc -l
69
/trino master$ git grep '.in(Scopes.SINGLETON)' | wc -l
675

[findepi]

"Test JDBC connection creations"

[findepi]

My IDE shows me a warning here.

'DriverConnectionFactory' used without 'try'-with-resources statement

[findepi]

I'd suggest writing this like

return new ConnectionFactory()
{
    private final DriverConnectionFactory delegate = new DriverConnectionFactory(new Driver(), config, credentialProvider);

and then adding

@Override
public void close()
        throws SQLException
{
    delegate.close();
}

at the bottom

[kokosing]

My IDE shows me a warning here.

Thank you. I had this disabled somehow.

[findepi]

USING may prevent JOIN pushdown, which looks like something you want to test for #6781

BTW it would be good to assert the pushdown actually happened.
It won't happen under default configuration due to lack of stats, right?

[kokosing]

I will add something with even more easier predicate and without a predicate at all too. I don't want to go very deeply about what is supported and what is not.

[findepi]

add SHOW STATS

also, we should test with a connector that supports table stats

[kokosing]

also, we should test with a connector that supports table stats

SHOW STATS is interesting as it is using JDBI which double closes connections. However I would prefer not to use postgresql connector here. Also using different connectors may have different number of opened connections. Maybe I should not count them at all?

[findepi]

java.lang.AssertionError: 
Expecting:
 <14>
to be less than or equal to:
 <11> 

[kokosing]

I think it is related to reusing connections.

[findepi]

assert that connectionCreations is empty.

also, rename the field to openConnections

[kokosing]

It is verified after setup of test class and after each test already.

[ssheikin]

This is a first pass of review and comments which I left here mostly for myself for the second path :)

[ssheikin]

I'm not really follow why cache has to be represented as map. Looks very similar to how Set is backed by Map, but I'd say it's not necessary abstraction here.

Chain of not very structured thoughts:
What if we try to add something to map? Would it mean that map is readonly? Logically it leads that Cache has to implement Map interface (too when readonly).

[kokosing]

You need to reverse your thinking. It is not cache represent as map, it is a map that is presented to you as cache. Internally that cache is implemented as map, basically I change the API here, behavior is same.

[ssheikin]

.remove I'd expect that connections holder (this class) is called pool

[findepi]

it's not a typical pool
This is effectively a queryId-keyd cache of 1-element pools.
It should be documented by keying by queryId is used here. This is not obvious especially that we don't reuse any "dirty" connections.

[ssheikin]

Looks like dirty path could be improved even more.
Do we have statistics how many connections fit to the dirty bucket?

[findepi]

"Reuse JDBC connection in JDBC connectors"

[findepi]

import statically

[findepi]

I think this should be enough:

Suggested change

	innerBinder.bind(ReusingConnectionFactory.class).in(SINGLETON);
	innerBinder.bind(ConnectionFactory.class).to(Key.get(ReusingConnectionFactory.class)).in(SINGLETON);
	innerBinder.bind(ConnectionFactory.class).to(ReusingConnectionFactory.class).in(SINGLETON);

but i cannot verify that since this code isn't exercised by tests

[kokosing]

It is tested by TestJdbcConnectionsCreation. They Key is a key here, otherwise you would get two different instances.

[findepi]

assert that now one is retained in the cache and one underlying has been closed already

[findepi]

it's not a typical pool
This is effectively a queryId-keyd cache of 1-element pools.
It should be documented by keying by queryId is used here. This is not obvious especially that we don't reuse any "dirty" connections.

[findepi]

Add separate test coverage for planning and execution

eg

assertOpenConnections(String sql, int expectedPlanningConections, int expectedExecutionConnections)

this would do

assertOpenConnections("EXPLAIN" + sql, expectedPlanningConections);
assertOpenConnections(sql, expectedPlanningConections + expectedExecutionConnections);

-- this would help understand why we're not reaching 1 connection per query, even with reuse

[kokosing]

:-)

Nice try. It is way beyond the scope of this commit (PR). Connector and even more ConnectionFactory has no notion about planning and execution. There is plenty of plumbing already and ask to test SHOW STATS for a real connector requires even more plumbing. Let's do go crazy with feature requests :)

[kokosing]

Extracted #14732

[kokosing]

I am going to reimplement this a bit. Having this "dirty" thing is very fragile, it is hard to tell what connection and when will be reused. Instead I will make part of the implementation of JdbcClient only for metadata queries.

[kokosing]

I think it is related to reusing connections.

[kokosing]

:-)

Nice try. It is way beyond the scope of this commit (PR). Connector and even more ConnectionFactory has no notion about planning and execution. There is plenty of plumbing already and ask to test SHOW STATS for a real connector requires even more plumbing. Let's do go crazy with feature requests :)

[kokosing]

I will add something with even more easier predicate and without a predicate at all too. I don't want to go very deeply about what is supported and what is not.

[kokosing]

My IDE shows me a warning here.

Thank you. I had this disabled somehow.

[kokosing]

It is verified after setup of test class and after each test already.

[ssheikin]

move it top.

[kokosing]

It matches the order in which fields are defined now.

[ssheikin]

is it a possible situation that connection returned from the cache is already closed? Is it a harmful?

[kokosing]

I don't think it is possible.

[ssheikin]

Duration.ofSeconds(5) -> const FOREVER = Duration.ofDays(5)
10 -> const VERY_LARGE = 1000

[ssheikin]

If I understood correctly Connection ignored is not yet closed, but connectionFactory.openConnection(ALICE) tries to open new connection in the same session.

Does it mean that ReusableConnectionFactory is designed not only for metadata retrieval, but a general implementation? (I guess yes)
Is it a real situation? (I guess not for metadata)
Does it mean that Metadata retrieval is parallel? (guess no)

[kokosing]

The point is that JDBC connection are so-so thread safe. So it is better not to share a single connection between different threads. Also the next step is to verify we don't open nested connections in the same thread.

Yes. Yes. Not for the same Trino query.

[ssheikin]

what if
Duration.ofMillis(0) ?
10 -> 0 ?
Is it covered by tests?

[ssheikin]

not related

[ssheikin]

What does session.getQueryId() returns for ALICE?

looks like this heavily relies on that metadata requests are not parallel.

and this test could be extended:

    @Test
    public void testConnectionIsNotShared()
            throws Exception
    {
        try (MockConnectionFactory mockConnectionFactory = new MockConnectionFactory();
                ReusableConnectionFactory connectionFactory = new CacheBasedReusableConnectionFactory(mockConnectionFactory, Duration.ofSeconds(5), 10);
                Connection ignored = connectionFactory.openConnection(ALICE)) {
            connectionFactory.openConnection(ALICE).close();
            assertThat(mockConnectionFactory.openedConnections).isEqualTo(2);
        }
    }

[ssheikin]

may be close it explicitly?

[kokosing]

That is the point that connection is still open.

[kokosing]

AC, PTAL

Notice that this is based on #14749

[kokosing]

consider previous connection to be closed and add test

putIfAbsent?

[kokosing]

It matches the order in which fields are defined now.

[kokosing]

I don't think it is possible.

[kokosing]

The point is that JDBC connection are so-so thread safe. So it is better not to share a single connection between different threads. Also the next step is to verify we don't open nested connections in the same thread.

Yes. Yes. Not for the same Trino query.

[kokosing]

That is the point that connection is still open.

[hashhar]

still need to see this in detail

[hashhar]

Test something like:

aliceConnection1 = connectionFactory.openConnection(ALICE);
aliceConnection2 = connectionFactory.openConnection(ALICE);
aliceConnection1.close();
aliceConnection2.close();

[kokosing]

Added as testSingleUserCanCreateMultipleConnections

[kokosing]

AC

[hashhar]

assumes connections are going to be acquired from a single thread - this is probably true for metadata queries.

To elaborate this assumes that metadata queries happen in single-threaded manner so for same queryId we'll only ever have 1 connection.

[kokosing]

It is not necessary. This factory can be used by multiple threads. The point is that only one connection can be stored for future use and no connection is not shared between threads.

There are no issues if two threads wants to create a connection for same queryId or single thread that wants to create two connections. This factory is not optimized for such use case so hit rate will be low, but there will be no regression and no correctness issues.

[hashhar]

Test something like:

aliceConnection1 = connectionFactory.openConnection(ALICE);
aliceConnection2 = connectionFactory.openConnection(ALICE);
aliceConnection1.close();
aliceConnection2.close();

[kokosing]

AC

[hashhar]

Looks good to me (for the goal of connection reuse only for metadata queries).

[kokosing]

All @findepi comments got addressed. I am happy to address more in follow up PRs.

[colebow]

This needs to be documented. Will submit a follow-up PR.