FWD » Core Development » Database

Teodor, mind the effort in #8388 of dropping dirty-share being stored in H2, but in persistence database.

Connection pooling was removed for the Meta and Dirty databases to simplify configuration and improve maintainability. Customer testing showed no measurable performance impact, the observed benefits were limited to standalone Java test cases. If you have hard evidence that adding the pooling back is improving performance for a customer project, then we can consider rewriting some code from 11288, however, make sure it is the connection reuse that helps and not some other side effect of using a DataSource.

Artur Școlnic wrote:

Connection pooling was removed for the Meta and Dirty databases to simplify configuration and improve maintainability. Customer testing showed no measurable performance impact, the observed benefits were limited to standalone Java test cases. If you have hard evidence that adding the pooling back is improving performance for a customer project, then we can consider rewriting some code from 11288, however, make sure it is the connection reuse that helps and not some other side effect of using a DataSource.

Maybe there was not so many FIND queries with dirty share in the tests used on #11288, but in a project, I see a substantial performance difference when keeping the connection in place vs opening and closing the connection everytime. Evidence is in #11502-5.

Of course there are side-effects using a DataSource. Since we are using C3P0, PreparedStatements are being cached, etc. There are H2 equivalents for these, but most important, we don't pay the price for opening and closing the connection everytime.

If it helps a customer application, I am ok with adding it back, just make sure to inherit the configuration of the dirty db from the tenant private db.

Can you take into the patch? Is a reasonable change?

It is good, but the used config for the data sources is the default one, it needs to use the tenant specific one to avoid cases where the persistent db has 100 connections and the dirty db (which, as you found, can be just as heavily used) has 5, thus becoming a bottleneck.

There is a list of c3p0 parameters which should not be copied from primary database, like minPoolSize, initialPoolSize, maxConnectionAge, etc.
maxPoolSize is safe to be copied.

Constantin/Greg, what do you think? We should copy only the safe parameters from primary database, or we should make these configurable from directory?

A quick reminder about the question from #11502-12.

#11502 is not a critical issue, but the performance is a bit worse, about 100% more time spent on dirty share logic because of #11288.

Teodor, the shared db has a directory config, it can be used for the dirty shared db. The tenant private databases have their config in the landlord database, it is not in the directory, that same config needs to be used for the dirty tenant databases. We don't need dirty specific configs, just use the corresponding primary db ones.

I don't think cloning shared primary db / tenant primary db config because of #11502-12.

Dirty DB connections is definitely less than the number of active connections for PRIMARY database (it can either be shared or tenant config).

On single tenant projects, the used configuration is the "shared" one, where often, the parameters are fine-tuned for the main database. These parameters don't always fit for dirty share database. Default C3P0 configuration seems a good start, but it is possible in production to reach to the default maxPoolSize value (which is 15).

Generally speaking I would expect the dirty db to be less strained than the primary one due to the limited scope of the dirty db transactions, but dirty config still needs to reflect the workload expected by the primary db, maybe not a one-to-one copy of the config, like you mentioned. Anyway, limiting the amount of the config that must be provided by the client is always a good practice, I still think the dirty db config needs to be derived from the primary one rather than be explicitly set in directory or landlord db.

Artur tested with this branch and he confirmed that the testing on #10614 app has passed.