FWD » Core Development » Database

FWD creates a data source (connection pool) for each tenant, if one is deprovisioned, the data source is not cleared, this may result in connection pool leaking.

Just to be sure, deprovisioning a tenant means simply deleting it via tenant API?

Artur Școlnic wrote:

Just to be sure, deprovisioning a tenant means simply deleting it via tenant API?

In FWD terms, yes.

Stress testing this would mean to use the tenant admin APIs to add/delete the same tenant (but lets say with different DB configurations each time) and check how FWD manages the c3p0 threads and maybe other resources.

Indeed, at the moment when support for tenants was implemented, the operation of removing them did not take this aspect into consideration.

Basically, when the TenantManager deletes a tenant (deleteTenant()) or only one of its databases (deleteDatabaseFromTenant()), the DatabaseManager is asked to clear its tenantDbConfigs. I think this is the best place to inform JdbcDataSource to perform similar cleanup on its sources.

After this is implemented we need to verify whether any occurrences of the deleted tenant is still in memory (in some maps or other data structure). This may lead to leaks if the operation is repeated.

Review of 11235a/r16436

• JdbcDataSource.java, line 207: a typo: "Attemp(t)ed". BTW, I think we can make sources final.
• TenantManager.java, line 5: copyright year should be 2026.

• the Persistence.tenantData: the Persistence objects are long-lived instances, possible for the whole lifetime of the FWD server. If we do not clear tenantData, the instances of Tenant will continue to accumulate;
• the ConnectionManager is context local and should be collected when user context is gone. This should remove the tenantListeners as well. However, I think this is not happening. Please connect and disconnect a few times to confirm the ConnectionManager instances are indeed GC-ed;
• the P2OLookup objects are as well context local and they seem to be correctly cleaned when the user disconnects.

Not sure if it is relevant in these cases, but in certain broker modes, the contexts can be reused across appserver sessions, so a simple test case may not reveal the leak, I think we should take the safe approach and properly clean all the collections.

Ovidiu,
Constantin raised the question of data source resources retention, mainly the helper threads, just to be on the safer side, I added explicit close() to the removed data sources in order to release the resources and terminate the threads.

I realized that the datasource for dirty database (in-memory, h2 dialect) should also be processed (removed from sources and closed). Unlike meta (which is shared), the dirty databases are private.

Ovidiu,
I have a few questions:
1. Not all h2 databases are marked as temp, is this expected?
2. The dirty database is created from the default tenant database, does this mean that all the dirty databases are the same?

In order to implement dirty db clearing properly, I need a link between the dirty db and the tenant specific primary database that it is associated with, this is proving to be trickier than it should :)

Also, what about the _temp database, where the temp tables reside? Shouldn't it also be tenant specific and cleared on tenant removal?

Artur Școlnic wrote:

Ovidiu,
I have a few questions:
1. Not all h2 databases are marked as temp, is this expected?

Yes.

2. The dirty database is created from the default tenant database, does this mean that all the dirty databases are the same?

This one is for Ovidiu.

Artur Școlnic wrote:

Also, what about the _temp database, where the temp tables reside? Shouldn't it also be tenant specific and cleared on tenant removal?

_temp database is not tenant specific. Is the application's responsibility to manage it properly.

Ok, it makes sense, so we do need to clear only the data source for the dirty databases.
On tenant delete, this is not an issue, just keep all the urls in a Tenant collection, but on a single database delete (or change?) the associated dirty data source must be cleared also, only there is no association between them, and creating one is counterintuitive because the dirty tenant db is a copy of the default tenant db:

public Database toType(Type t)
   {
      ...
      // derived tenant databases will keep the reference to their tenant-default database 
      Database defaultDb = new Database(getDefault().getName(), t, local);
      defaultDb.socketAddress = socketAddress;
      ...
      return defaultDb.createTenant(name);
   }

Artur Școlnic wrote:

Ok, it makes sense, so we do need to clear only the data source for the dirty databases.

Do we have a dirty database instance in H2 for each tenant?

I would expect so. I am using just one tenant, so there are 2 dirty db, the default tenant and the explicit one.

Artur Școlnic wrote:

I would expect so. I am using just one tenant, so there are 2 dirty db, the default tenant and the explicit one.

Please test with two or more tenants.

• N + 1 sources for each (postgresql) permanent database (that is, one shared and N private tenants);
• N + 1 sources for in-memory (H2) dirty database. These will 'mirror' the above data-sources;
• 1 in-memory (H2) source for (shared) _meta table

When a tenant database is removed, the associated sources for permanent (postgresql) and dirty (h2) database should be closed and removed.
When the full tenant is dropped (maybe also when it gets deactivated) the full set of 2N + 3 data sources must de dropped.

Indeed, the URL looks more complicated than the database's name. That is because it also contains behavioural parameters. To get the proper string, we need to apply the same decorations as we do when the database is used a key when requesting a (new) data source (see getConnectionURL()).

Additionally, as Constantin noted, the _temp database has its own data source, but it is not related to tenants.

Ovidiu, please comment on #11235-19.

Constantin Asofiei wrote:

Do we have a dirty database instance in H2 for each tenant?

Yes. This is the architecture right now. The intention was to isolate the private data from the shared one. I did think of this, and I do not know if it would be possible to reduce the number of dirty databases to only one. We must be sure the data will not leak. If a tenant has some dirty records, we don't want those to interfere with the dirty records of the shared version of the table or from another tenant. Do we? But again, this MUST be tested. What if this leak happens in 4GL? That would be a serious issue: one tenant to see the uncommitted data of another tenant. But since in 4GL the data from different tenants is (apparently) stored multiplexed in same table, there is a chance that this might happen ☹️.

Artur Școlnic wrote:

Ovidiu, please comment on #11235-19.

As documented in javadoc of that method, the returned database preserves the internal information, returning a 'sibling' database. Assuming we have a logical database fwd with a tenant fwd_ten1. If you need the dirty version of a primary tenant database (local/fwd_ten1/primary (fwd) in our case), calling

db2 = db.toType(Database.Type.DIRTY)

db2.getDefault()

Artur Școlnic wrote:

Understood, thank you.
I got another question, using the tenant api, we can update a tenant, including the urls. On url update, I assume we should replace the old data source with a new one.

That is correct. I do not think we are correctly processing that request. If only the port from url is changed, the full data source should be re-created. The same for user, password, etc. This might complicate things.

Artur Școlnic wrote:

Understood, thank you.
I got another question, using the tenant api, we can update a tenant, including the urls. On url update, I assume we should replace the old data source with a new one.

Hm... changing the JDBC URL (or even just the user and password, or c3p0 settings) for the tenant is the same as deleting it and updating it - I mean, in FWD we need to cleanup all resources for the previous URL and create them for the new one.

I don't see how we can do this otherwise.

Ovidiu Maxiniuc wrote:

Review of 11235a / r16440

Nicely done! I think the code will work as expected.

Thank you, Ovidiu.
During testing I observed that almost all the collections are cleared when they should, the exception is tenantData and I suspect tenantListeners also (although I have no proof for that). I think the issue is that on tenant delete the tenant database is not actually disconnected and the cleanup code is not reached.

ConnectionManager.disconnect expects the logical db name, which can/should be the same for all the tenant databases, so disconnecting it seems wrong.
Ovidiu, Constantin,
Please advise.

This method is the implementation of DISCONNECT statement. Unfortunately, this is not documented in javadoc. Therefore, the parameter is expected the be the logical database name, as used in a CONNECT statement previously.

Most likely, you do not need to use that when disconnecting a tenant private database.

Ovidiu, I meant on tenant delete, do we need to disconnect the private database?

Well, that's a philosophical question :).
When a tenant is deleted, beside the effect on the database we discussed above, the users of that tenant are no longer valid. The clients authenticated with these users will have to be kicked out. Of course, a nice error message will be displayed.

Smoke-testing a customer app with 11235a passed.

Ovidiu Maxiniuc wrote:

Well, that's a philosophical question :).
When a tenant is deleted, beside the effect on the database we discussed above, the users of that tenant are no longer valid. The clients authenticated with these users will have to be kicked out. Of course, a nice error message will be displayed.

I will assume that it is the customer's responsibility to disconnect the the private database before deleting the tenant.
Since the testing passed with both a multi tenant and non tenant project, I think we can merge.

Do we need to port the changes to custom_build_line?

11235a was merged to trunk as revision 16453.