FWD » Core Development » Database

To upgrade the landlord database without re-creating it from scratch use:

alter table tenant_databases
      add column c3p0_max_stmts int4, 
      add column c3p0_min_pool int4,
      add column c3p0_max_pool int4,
      add column c3p0_acq_inc int4,
      add column c3p0_max_idle int4;

I will add the statement for other dialects, as I write an test them.

Ovidiu, what is the status of this task? I did not do a full review, but I took a look at 9831a and the implementation looks quite far along. What is left to do?

Isn't c3p0.break_on_fail always supposed to be true? I don't think we want customers to configure that.

The default value for all of these is null which means they will default to the c3p0 value of the default database.

However, the SQL statements are meant only for migration from previous revision of landlord database where these settings were not available. When the tenant database is configured, the admin can either use a specific value for the setting or skip it, and FWD will use the value from the default database, if one is set. Having a default value set at database level, would make this 'inheritance' more difficult, if not impossible.

BTW, here is an example of tenant database creation (using POST https://localhost:7443/admin/tenants/fwd/db API) which defines all the c3p0 values:

{
    "physicalName": "fwd",
    "logicalName": "fwd4",
    "url": "jdbc:mariadb://localhost:3306/fwd4",
    "username": "fwd_user4",
    "password": "^87#6)+hTb1@",
    "maxStatementsPerConnection": 10,
    "minPoolSize": 2, 
    "maxPoolSize": 8,
    "acquireIncrement": 3,
    "maxIdleTime": 100,
    "breakAfterAcquireFailure": false
}

Ovidiu, is this ready to be reviewed? I see that it's been placed at 90%.

• In Dialect.TenantSQL, the field name should be C3P0_MAX_STMTS instead of C3P0_MAXS_TMTS. This also means adjusting the places that use this field.
• In the same place (Dialect.TenantSQL), the javadoc of C3P0_BREAK_AFTER_ACQUIRE_FAILURE references max idle time instead of break on fail.
• In TenantDatabaseDescriptor, the javadoc of field c3p0BreakAcq has two starts (/**) and it references the wrong value (maxStatementsPerConnection).
• In TenantDatabaseDescriptor, the methods getC3p0BreakAcq() and setC3p0BreakAcq() are missing javadocs.

Please remove breakAfterAcquireFailure from the REST API, since we hard-code this setting to true in the runtime.

The breakAfterAcquireFailure was removed from REST API and landlord database.
The issues from note #9831-15 (those valid after 1st operation) were addressed.

The branch was rebased and now it is at revision 15897.

Radu, please do a quick review when you have the time.
Thank you!

Meanwhile, I am putting the branch to the test using the the hotel, webGUI navigation and harness smoke-tests of a customer and the unit-test of another large customer application.

Ovidiu Maxiniuc wrote:

Radu, please do a quick review when you have the time.

I think 9831a is alright. I don't see any more issues.

While the testing phase is in progress, maybe we can get one final look from Eric? Just to have a second final opinion.

I wanted to confirm that the breakAfterAcquireFailure is no longer needed in the directory.xml and is now being forced to true at runtime.

The documentation for c3p0 is still referencing it and states the default is false.

https://www.mchange.com/projects/c3p0/#configuring_recovery

The breakAfterAcquireFailure is no longer settable. It is always active (that is, its value is true).
For more details, see the discussion on this matter at the end of #9736.

This was a breaking change for deployed environments because the upgrade of the landlord schema changes were not automated. A manual script or a complete teardown and rebuild of the landlord database is not a viable workaround. There needs to be an automated method to upgrade schema for the landlord if future changes are made.

I am sorry for the trouble.

The SQL statements stand in notes 6 (for PgSQL) and 8 (for H2). I adjusted them by dropping the support for breakAfterAcquireFailure. If that was already created, there is no problem, it will just be ignored. The upgrade was not implemented since we are still in development phase therefore the additional code is not justified (from my knowledge there is no production server on which it would have been executed).

I will try to find a smoother transition if similar upgrades will be necessary.

The concern is from Cloud Ops because they do not have access directly to the databases in production. On our local installs we can rebuild the landlord or apply a SQL script directly which is now we resolved the changes this morning.

This is not possible in production and Cloud Ops wondered why the schema was not auto updated similar to how you initially create it during a new landlord population. Their concern is if this happens again in the future if you do not auto upgrade the landlord schema it would result in a system down scenario with users not able to login and the server failing to startup due to these types of changes to the landlord DB.

Testing now that all three RedMine issues are in place with the directory.xml removal of breakAfterAcquireFailure
https://proj.goldencode.com/issues/9002
https://proj.goldencode.com/issues/9736
https://proj.goldencode.com/issues/9831

Results are as expected with no signs of the Server failing and API’s are responsive after the PostreSQL connection is reestablished.
The Admin API allows for a login and retrieval of existing Tenants with the PostreSQL connection down.
Once PostgreSQL is started again Admin API and the Server Agents reestablish connections and API calls are successful as expected.