FWD » Core Development » Database

OE knows whether a table is shared (default tenant) or is multi-tenant. In OE, setting the tenant using SET-DB-CLIENT() enables access to the tenant-specific data in multi-tenant tables but it doesn't disable access to shared data (default tenant). Is that correct?

In other words, if shared data is in use in the 4GL code, it is likely that this usage is intermingled. The shared data will be accessed while a non-default tenant is set. I think we don't handle this properly right now. At least I don't recall us doing anything to address it.

• We need to know which tables are shared and which are multi-tenant. That suggests additional data brought through into DMO annotations.
• We need to differentiate the access to shared tables and handle that from the default tenant database, which means having 2 different JDBC connections active at the same time for the same logical database.
• It would likely have database-level transaction implications because OE will consider this a single database while we have two databases in use.
• This may impact caching and other state along the way.

Greg Shah wrote:

OE knows whether a table is shared (default tenant) or is multi-tenant. In OE, setting the tenant using SET-DB-CLIENT() enables access to the tenant-specific data in multi-tenant tables but it doesn't disable access to shared data (default tenant). Is that correct?

I don't know if this got a response or not, but the statement is correct. A table that is not marked as multi-tenant can be accessed by all tenants (after using SET-DB-CLIENT()) and it acts as a shared storage space. This means that queries will return the same results no matter the tenant that is currently owning the context.

Current status:

While the private tables in a tenant enabled database works correctly, the shared tables are still an issue. At the moment, the buffer for such a table is aware of its type (the information is taken from metadata, dmoInfo). However, when initialized and also in subsequent operations the database is provided by its persistence object, which is configured in the current context to use the connection to physical database.

I am experimenting at this moment a solution with a logical flag for this tables, which is designed to force the persistence to dig the right connection. Beside the obvious performance issues, there are some other details I am working around one-at the time.

• each Database object should have a reference to default Database. In case of a physical tenant database the default is the parent logical database. In case of shared / non multi-tenant databases, it is a self reference;
• each record buffer now has the knowledge (in its own DmoMeta object) whether it belongs to a shared or a private table (there is a flag for with default, too);
• as result, when computing the database for a buffer these flags are used. This piece of code got a bit more complicated;
• my idea is that each query component, when processing a buffer, will use the buffer flags to infer the physical database: the common buffers will use the 'default' physical database, while the shared buffers will use the database which is currently set in current context. The default physical database can be obtained using the defaultTenant member of Persistence.
• one issue is that, for tables from different physical databases, the server-side optimizations are no longer possible, even if they appear to be on same logical database. Some dialects might offer some support in this regard, but this is not a common feature so we cannot go this way. As result, when checking if a compound query with multiple buffers can be optimized, this aspect must also be taken into consideration.

For the moment, the solution is not working, the data is still always fetched from the tenant database, even for tables which are shared.

New update.

I am advancing with identifying and choosing the right physical database when some API from persistence object is invoked. Things seems to get going, but there are some method for which
the target database is really unknown,

For some like: commit(), rollback(), flush(), things seem clear, both context are affected.

• scroll(String[] entities, String fql, Object[] values, [...])
• list(String[] entities, String fql, Object[] values, [...])
• quickLoad(RecordIdentifier<String> ident, boolean refresh)
• executeSQLQuery(String sql, Object[] args)
• getSingleSQLResult(String sql, Object[] args)
• executeSQLBatch(List<String> sql, boolean noFlush)
• lock(LockType lockType, RecordIdentifier<String> ident, [...])
• queryLock(RecordIdentifier<String> ident)
• deleteOrUpdate(String fql, Object[] values, [...])

Note that in some cases the entities or RecordIdentifier might offer a bit of information for the right database, but not always. The RecordIdentifier may contain only the table name, even if this is not impossible to get the database, the process is complex.

Some of the above methods have only a fql / sql statement which makes it impossible to select the right database. A sole solution I see now is to request the information from the caller. Locally, there may be some information on the respective tables. The needed flag is DmoMeta.multiTenant.

Eric and Radu: please review.

I assume the revisions that need reviewing are 15526-15532 inclusively. Rev. 15533 seems to be with things from the trunk, but please correct me if I'm wrong.

Radu Apetrii wrote:

Rev. 15533 seems to be with things from the trunk, but please correct me if I'm wrong.

Or not. Next to the revision number there was a [merge], and I assumed it came from trunk, but now I'm quite confused.

Radu Apetrii wrote:

Or not. Next to the revision number there was a [merge], and I assumed it came from trunk, but now I'm quite confused.

I do not know why is that [merge] on last revision. Maybe because of the rebase. The branch is based on trunk r15525 committed by Stanislav on 2024-10-30.

I started a new rebase operation, but there are several large conflicts I cannot manage it on devsrv01, so I'll find a solution and keep you updated.

I have rebased the branch successfully. The current revision is 15551, based on trunk 15543.
Ready for review, but please ignore the missing H entries, I intentionally delayed adding them to avoid conflicts while rebasing.

Code review 9057a/15544-15551:

General question: throughout the changes, there are a number of "TODO 9057" tags added. Some of these seem important to the functionality of the shared/private table implementation. What is the plan to address these?

BufferImpl: there are a number of methods which return immutable logical constant instances (they used to return logical.of({true|false}), now they return logical.{TRUE|FALSE}). Is this safe? We don't know what the caller is doing with these instances. If, for instance, a caller assigns the result of a buf:RAW-TRANSFER(...) call to a logical variable, then later re-assigns that variable, wouldn't that result in a call to logicalConstant.assign, which would throw UnsupportedOperationException? It seems OK to use logical constants as parameters to method calls and as return values from private/protected/package-protected methods (as long as the callers are careful with them), but not necessarily from public methods.

BufferManager.reclaimPendingKeys: there is a TODO comment here to "iterate both" when getting the identity manager. I assume this means get the identity managers of both the tenant and shared contexts. However, we don't reclaim keys from permanent databases, only from the temp-table database, so I think this TODO is unnecessary.

DatabaseManager: a number of multitenant-related methods have incomplete javadoc, in that the return type is "n/a".

Persistence: New static variables need javadoc. The signatures of some public methods have changes. At least one project (our oldest) may use these methods. I think the latest iteration of that project no longer does this, but please confirm that project's build is not broken by these changes. A normal regression test run should suffice.

RecordBuffer:

• There is a TODO that the persistence context may change when the tenant changes. We need some test cases to show how this might work.
• In the upgradeLock method, there is a comment: "the records created by the default tenant are volatile. No locking in this case." What does this mean?

Session: looks like some debug code left over at the end of this class?

Overall: I don't see anything obviously wrong with the implementation (with the caveat about the TODOs above). But there is a lot of change here and it is hard to know what I may have missed in my review. We're going to need full regression testing.

Eric Faulhaber wrote:

Code review 9057a/15544-15551:

Thank you for the quick review.

General question: throughout the changes, there are a number of "TODO 9057" tags added. Some of these seem important to the functionality of the shared/private table implementation. What is the plan to address these?

These are problematic places where the context information was not readily/easily accessible. These are the places I reported in my note #9057-9.

BufferImpl: there are a number of methods which return immutable logical constant instances (they used to return logical.of({true|false}), now they return logical.{TRUE|FALSE}). Is this safe? We don't know what the caller is doing with these instances. If, for instance, a caller assigns the result of a buf:RAW-TRANSFER(...) call to a logical variable, then later re-assigns that variable, wouldn't that result in a call to logicalConstant.assign, which would throw UnsupportedOperationException? It seems OK to use logical constants as parameters to method calls and as return values from private/protected/package-protected methods (as long as the callers are careful with them), but not necessarily from public methods.

The result is semantically identical. The logical.of() returns one of the constants. The changes are merely an optimization, by inlining the constant directly and avoid a method call.

BufferManager.reclaimPendingKeys: there is a TODO comment here to "iterate both" when getting the identity manager. I assume this means get the identity managers of both the tenant and shared contexts. However, we don't reclaim keys from permanent databases, only from the temp-table database, so I think this TODO is unnecessary.

Right! Removed.

DatabaseManager: a number of multitenant-related methods have incomplete javadoc, in that the return type is "n/a".

Thank you for noticing these. I think they were inherited from a previous revision. Nonetheless, they had to be fixed. I updated the javadoc.

Persistence: New static variables need javadoc.

Done.

The signatures of some public methods have changes. At least one project (our oldest) may use these methods. I think the latest iteration of that project no longer does this, but please confirm that project's build is not broken by these changes. A normal regression test run should suffice.

OK. Good to know. We will do more tests, anyway. If there is a problem with the method signatures we will now early, at conversion/build time.

RecordBuffer:

• There is a TODO that the persistence context may change when the tenant changes. We need some test cases to show how this might work.

I did think ahead with this but the implementation will probably require a bit of improvements. These objects need to be updated when the tenant changes, practically re-connecting them to other physical database.

• In the upgradeLock method, there is a comment: "the records created by the default tenant are volatile. No locking in this case." What does this mean?

This is about the tenant table without default support. This is really strange: 4GL allows creation and deletion of these records, but not saving them (an error is raised on flush/index them).

Session: looks like some debug code left over at the end of this class?

Correct. Removed from branch.

Overall: I don't see anything obviously wrong with the implementation (with the caveat about the TODOs above). But there is a lot of change here and it is hard to know what I may have missed in my review. We're going to need full regression testing.

I will make sure of it.

I have committed a new revision of 9057a (r15574, was rebased to trunk 15560).

It contains clarifications for all situations for cases where the different contexts can be chosen which were mentioned in #9057-9. I will do a basic regression test sets to be sure

• detection of these queries (which were assumed to be run in single database but are actually cross-database);
• split the query in multiple unoptimized queries and execute them cross-database, using specific compound queries and/or client-side WHERE clauses;
• caching at different levels is probably affected so additional components might be added to keys.

Update on my latest effort to get done the last known multiple-tenant issue described above.

The compound queries which are constructed from independent components for each buffer emitted at conversion time for which FWD usually does an effort to combine / optimise so that they can be executed as a single JOIN SQL query is naturally handled. The new Database objects contain the necessary information and the latest implementation of Buffer.getDatabase() will make sure the correct objects are returned so that the join of two tables which are normally in same logical database is prevented if they are not mapping tables from the same physical database.

The second type of problematic queries are caused by the usage of CAN-FIND() function inside WHERE predicates. For example a rather simple query like:

FIND FIRST test WHERE CAN-FIND(mt_with_default WHERE test.f1 + 10 EQ mt_with_default.f1) NO-ERROR.

new FindQuery(test, 
               "(select count(mtWithDefault.recid) from MtWithDefault as mtWithDefault where test.f1 + 10 = mtWithDefault.f1) = 1", 
               null,
               "test.f1 asc")
      .addExternalBuffers(mtWithDefault)
      .silentFirst();

When the test and mt_with_default tables are not in the same physical database (for example, when one is public/shared and the other tenant-private), the above code will fail. Instead, it must be converted into a cross-database query which should look like this:

new FindQuery(test, 
            (String) null, 
            () -> new FindQuery(mtWithDefault, "? = mtWithDefault.f1", null, "mtWithDefault.recid asc", new Object[] { plus(test.getF1(), 10)}, LockType.NONE).hasOne(),
            "test.recid asc")
      .silentFirst();

• unique (this case) and not unique can-find are emitted slightly different;
• the outer WHERE predicate may be a more complicated expression, not only the call to CAN-FIND() function;
• the predicate of the inner SELECT is 'transferred' to a new FindQuery object which will be constructed in a manner which is a bit similar to execution of dynamic queries;
• there may be multiple calls to CAN-FIND(). Each one needs to be converted to its own FindQuery. They can also be nested;
• the WHERE predicate of CAN-FIND can refer fields of the outer/main query. As seen in this example, the expression in which they occur must be extracted (back) as SUBST parameters for the inner query. I do not know how to 'generate' back the Java code for this (in this example, converting test.f1 + 10 string to plus(test.getF1(), 10) code). They seem similar to dynamic queries, but I do not want to write a new, similar engine;
• the example above uses a simple FIND FIRST query, but the problem applies in case of FOR loops, too.

The first half of the bullets above are not difficult, but the last ones are non-trivial. I am also trying to isolate these query transformation (not only at FQL level) so that the new code will not interfere with the normal processing of the FQL.

Are you expecting conversion to be specific to MT in this case? It would be the first thing that was hard coded in the converted result that requires MT to be active. That is something we want to avoid.

If it can't be avoided, we could emit both MT and ST (single tenant) variants and switch between them. Or we could emit a more generic result that the runtime can "morph" to the right version as needed.

a new set of FindQuery APIs which would accept both tuple of (fql, client-where, args). I think we can keep the current constructors and only inject after the addExternalBuffers() with a new method which provides the alternative query.

I love this. I assume it only happens when we detect that at least one table is multi-tenant? Or perhaps when we have a flag set at conversion time. We don't want this to change the conversion for ST customers.

Will the runtime still support running in single-tenant and multi-tenant mode without the need for a conversion when switching modes? This is vitally important as the same code base may run in a cloud (multi-tenant) environment, a developers (single-tenant) environment, or a hybrid mode (where code is hosted in the cloud, but running in single-tenant mode).

Changes made to the directory.xml would enable/disable multi-tenancy (https://proj.goldencode.com/projects/p2j/wiki/Multi-Tenancy#Enable-Multitenancy) and the converted code would handle both use-cases?

Carson Mader wrote:

Will the runtime still support running in single-tenant and multi-tenant mode without the need for a conversion when switching modes? This is vitally important as the same code base may run in a cloud (multi-tenant) environment, a developers (single-tenant) environment, or a hybrid mode (where code is hosted in the cloud, but running in single-tenant mode).

Changes made to the directory.xml would enable/disable multi-tenancy (https://proj.goldencode.com/projects/p2j/wiki/Multi-Tenancy#Enable-Multitenancy) and the converted code would handle both use-cases?

Yes, this is a core objective that will be met. The proposed change was made with this exact idea in mind.

Greg Shah wrote:

a new set of FindQuery APIs which would accept both tuple of (fql, client-where, args). I think we can keep the current constructors and only inject after the addExternalBuffers() with a new method which provides the alternative query.

I love this. I assume it only happens when we detect that at least one table is multi-tenant? Or perhaps when we have a flag set at conversion time. We don't want this to change the conversion for ST customers.

Exactly, the additional alternative execution plan will be injected based on the annotations in the .df file. If both buffers/tables are shared or both are tenant-private, the code will be the unchanged as we are facing an intra-database join. Only the rare cases where they MAY belong to different physical databases will require conversion changes.

I am glad we discussed and now I have a clear goal for this last part. Thank you!

Yesterday I implemented the runtime support for the setMultiTenantAlternative() to be method injected in case of possible cross-database necessity. I used several hand-written test-cases and it showed the solution is working. The changes were committed to 9057a as r15584.

Today I continued work and attempted to generate the setMultiTenantAlternative() as required, injected to cross-database queries. In tests, the conversion generates the parameters, only in very strict conditions, but the result is not yet compilable. Although the new FQL and client-side query looks correctly generated, they are not attached at the right places. This will requires a couple of hours of work to be fixed. I committed these changes in 9057a as r15585.

I think I have finished the multi-tenant specific implementation. The latest commit is 15586 in branch 9057a.
Since the changes are big, heavy testing is necessary. I've made following plan:

• NO changes in the converted code (including DMOs and DDLs);
• NO changes in the output/GUI during runtime;
• small performance degradation (my estimation 1-2%). Please report if this is noticeable.

I also would appreciate if the code is reviewed in the meantime. Note, however that the H entries are intentionally skipped to make the rebase process easier. I will add them at the last rebase as they do not interfere with the logic of the code.

Thank you!

I attempted to patch 7156c with the changes from 9057a, but did not succeed, the patch is over 10k lines, 7156c and trunk already have major differences, picking only the ones from 9057a is not an option, I propose to convert the largest app with trunk or at least a 7156c that is as close to trunk as possible, make a baseline, and then convert with 9057a.

Eric Faulhaber wrote:

Ovidiu, don't you need to rebase the branch?

I've just done the rebase. The current revision is 15595.
I will add the H entries for all affected files and describe the flow for the conversion in respective TRPL files. Although these will skew the line numbers for the files, the logic content will remain unchanged.

Ovidiu, should I test 9057a? There is also a 9057b, that is why I am asking.

Ovidiu, I started looking into 9057a, but I failed running ant deploy on the ChUi regression tests because it uses some direct Java access whose API is no longer compatible with 9057a:

starts with Persistence persistence = PersistenceFactory.getInstance(DB_NAME);

• txOpen = persistence.beginTransaction(); - no suitable method found for beginTransaction(no arguments)
• rs = persistence.executeSQLQuery(sql, parms); - error: method executeSQLQuery in class Persistence cannot be applied to given types;
• persistence.commit(); - error: no suitable method found for commit(no arguments)
• persistence.rollback() - error: no suitable method found for rollback(no arguments).

This is done in a very intuitive context: retrieving a persistence, beginning a transaction, running a query and committing (or rollback in case of an exception).

I either think that the old API should be brought back with some reasonable defaults (if possible) or let me know what should be done to properly adapt this code. Mind that there are other applications using direct-Java access (a large GUI customer for instance that also has a mutable database which is accessed on a low-level basis).

Alexandru,
Thank you for reporting this.
I encountered similar issue when I did some tests with the large application which Eugenie is converting and I added specific APIs, but they are NOT MT-compatible. I documented them in the javadoc. I will add the ones you reported, as well, in same conditions.

Done. I did a quick smoke test to check at least it compiles correctly.
I am going on with tests as described in #9057-29.

LE: Committed revision 15596.

Added TRPL documentation on MT processing.
Committed revision 15597.

Ovidiu, please remove this debug line:

src/com/goldencode/p2j/persist/orm/SessionFactory.java:381: System.out.println("Closing idle sessions... " + factory.reclaimable + " ...NOP");

Ovidiu, I am sorry, but I missed:

error: cannot find symbol
    [javac]             if (txOpen && persistence.isTransactionOpen()) {

Testing ETF, in the conversion the only change is a new line which no longer exists between the var def and the forEach block:

                  AdaptiveQuery query0 = new AdaptiveQuery();
                  forEach(query0, "loopLabel1", new Block((Init) () -> 

Otherwise, ETF testing passed.

• I addressed the issues reported by Constantin in note #9057-38 and Alexandru in #9057-39. Committed revision 15598. They do not need re-testing;
• Eugenie reported the conversion was successful, but the same skipped empty lines between the declaration and body of the query can be seen. He did not investigate all occurrences, I will do the full-search to confirm;
• I run the unit-tests for the above application (with baseline code) and it passes. I will also do a GUI navigation. I have already imported the database last night;
• I am waiting for Alexandru for finishing the tests with the remaining application.
• Then, I am going for another rebase and then add the H entries while waiting for final review.

• persistence.flush();
• Long pk = (Long) persistence.nextPrimaryKey("...");
• DataObjectFactory.getInstance(persistence.getSession()) or simply Session session = cp.getPersistence().getSession();
• p.lock(LockType.NONE, identifier, true);
• long batchNum = persistence.getSingleSQLResult(batchNumQuery, null);

Also, there were no conversion differences except some empty lines that were removed.

Alexandru Lungu wrote:

I was successful in converting and compiling some part of the direct Java code. However, I got to a point where there are way more problems on the API usage:

• persistence.flush();
• Long pk = (Long) persistence.nextPrimaryKey("...");
• DataObjectFactory.getInstance(persistence.getSession()) or simply Session session = cp.getPersistence().getSession();
• p.lock(LockType.NONE, identifier, true);
• long batchNum = persistence.getSingleSQLResult(batchNumQuery, null);

Also, there were no conversion differences except some empty lines that were removed.

Ovidiu, are you able to resolve these and get the last piece of the regression testing (the ChUI app) done today? I think Alexandru will be gone for the day by the time these signatures are added back. These changes are additive and should not affect the other regression testing which already has passed.

I've just done that. Committed revision 15599.

I wanted to test them but for some strange causes, the project failed to compile (h2 compatibility, although the jar was not updated). It was OK after a clean build.

Ovidiu Maxiniuc wrote:

I've just done that. Committed revision 15599.

I wanted to test them but for some strange causes, the project failed to compile (h2 compatibility, although the jar was not updated). It was OK after a clean build.

Can you run the ChUI runtime testing?

Never done, in the new format. I will try to.

Code review 9057a/15578-15598:

Big update! In part, this is because I have re-reviewed the older revisions I reviewed previously, to get the whole picture, as well as the latest changes. So far, the single-tenant regression testing results give me some confidence, but this will need to be well-tested in multitenancy mode.

Some comments/questions:

• I reviewed the changes, but it is hard to review for possible missing changes, such as places where the more specific persistence context or other resource must be retrieved, to differentiate between shared and tenant-specific tables. I understand you tried to hunt all these down, but this is a bit of a blind spot in my review. The approach I would have taken (which it seems like you did as well) would have been to change the signature of the methods to retrieve these resources and to let the compiler tell me all the places that needed to be reviewed and possibly changed. Then add back the old signatures to address external dependencies in the applications which use these APIs directly. Is that basically how you went about this?
• As discussed in recent standups, please add the missing header entries.
• I know you do not yet have the environment to confirm this, but can I assume your expectation is that #9362 should be resolved by these changes?
• How are we dealing with dirty share changes w.r.t. private vs. shared updates?

Admin Interface: the changes in this area seem OK to me, but this was not included in the regression test plan. Have you done any ad-hoc testing of the changes in this area? If not, we will need to loop back around and test this functionality, but I don't want to delay the immediate delivery of 9057a for this.

• Is the wrapping of certain existing constructs in an isRuntimeQueryMode() check related to this MT functionality, or is that unrelated cleanup?
• There may be some unnecessary casting, specifically, in places where we do a narrowing object assignment to a TRPL variable. I think the auto-boxing in TRPL should handle these cases, but this is not an important change to make, since the code works as written.

AbstractQuery: is there a reason the setMultiTenantAlternative method is implemented at this level, only to throw an exception? It is required to be implemented by subclasses to satisfy the P2JQuery interface. If the method is implemented in this class, it will hide the fact that a meaningful implementation in a concrete subclass might be missing. This is something that should be caught at FWD compile time.

ConnectionManager: there is some debug printing to System.err left behind.

Persistence.getContext(Class[]): in the event of a mixed set of entities (i.e., some shared, some tenant-private), it is not clear to me what is supposed to happen. It seems we will always get the private context in that case, though a warning will be logged (same outcome with an empty array of entities). Is that correct, and is this a valid state? Similar question for PreselectQuery.isSharedContext.

RecordBuffer.updateTenant: not something to address right now, but I am a little concerned about the performance implications of cycling through all active buffers when the tenant changes. Perhaps an alternative implementation would be to maintain a counter of tenant changes in the Persistence object and in the buffers, and only if the buffer's counter is lower than that of the Persistence object, update the buffer's state. Not ideal either, since it would require checks where the context is used within the buffer. So, let's put a pin in this until we can profile and figure out whether this actually represents a bottleneck.

RemoteIdentityManager.setPersistence and RemotePersistence.createLockManager: why is Persistence.META_CTX used in these cases (also in the sequence handlers' safeContains methods)?

getTenantType methods in P2OAccessWorker and P2OLookup need javadoc.

Eric Faulhaber wrote:

• I reviewed the changes, but it is hard to review for possible missing changes, such as places where the more specific persistence context or other resource must be retrieved, to differentiate between shared and tenant-specific tables. I understand you tried to hunt all these down, but this is a bit of a blind spot in my review. The approach I would have taken (which it seems like you did as well) would have been to change the signature of the methods to retrieve these resources and to let the compiler tell me all the places that needed to be reviewed and possibly changed. Then add back the old signatures to address external dependencies in the applications which use these APIs directly. Is that basically how you went about this?

Yes, something like this. The main questionable resource in persistence is now the context. Once I switched to contexts as context local array, all occurrences of the old filed were 'in red'. Well, this was not the first approach, but the idea is the same. Then it was just a matter to analyse each of these occurrences and decide how to access the expected context.

• As discussed in recent standups, please add the missing header entries.

I am doing this right now. I will post a note with the revision number.

• I know you do not yet have the environment to confirm this, but can I assume your expectation is that #9362 should be resolved by these changes?

It is possible. There are a lot of changes here. I am guessing the problem is switching the buffers to use the new context after a tenant change. You 'complained' this is a performance issue, but is is necessary (see below, updateTenant).
If this is not the case, I will need the MT setup to debug it.

• How are we dealing with dirty share changes w.r.t. private vs. shared updates?

These are persisted to a kind of temporary table, there is no PRIVATE context for them. All happens in SHARED context, in a manner similar to temp-tables and meta database.

Admin Interface: the changes in this area seem OK to me, but this was not included in the regression test plan. Have you done any ad-hoc testing of the changes in this area? If not, we will need to loop back around and test this functionality, but I don't want to delay the immediate delivery of 9057a for this.

database_access.rules:

• Is the wrapping of certain existing constructs in an isRuntimeQueryMode() check related to this MT functionality, or is that unrelated cleanup?

These <actions> are specific to runtime (cache of query class object). The test is also performed in side the TRPL defined function, but that requires the parameters to be evaluated in advance. My problem was that the evaluation failed with in-progress code, but then I kept it as a local optimization.

• There may be some unnecessary casting, specifically, in places where we do a narrowing object assignment to a TRPL variable. I think the auto-boxing in TRPL should handle these cases, but this is not an important change to make, since the code works as written.

Possible, I usually try to rely on autoboxing, but TRPL sometimes will not process some constructs. If you give more details (file/line) I can test whether the cast can be dropped.

AbstractQuery: is there a reason the setMultiTenantAlternative method is implemented at this level, only to throw an exception? It is required to be implemented by subclasses to satisfy the P2JQuery interface. If the method is implemented in this class, it will hide the fact that a meaningful implementation in a concrete subclass might be missing. This is something that should be caught at FWD compile time.

You are right here. I did miss two places. I will add the code after I test it. They will not be accessible from ST environment so it cannot be regression tested, but reviewed. Thank you for spotting this.

ConnectionManager: there is some debug printing to System.err left behind.

Dropped.

Persistence.getContext(Class[]): in the event of a mixed set of entities (i.e., some shared, some tenant-private), it is not clear to me what is supposed to happen. It seems we will always get the private context in that case, though a warning will be logged (same outcome with an empty array of entities). Is that correct, and is this a valid state? Similar question for PreselectQuery.isSharedContext.

This method should be called when FWD tries to guess and validate the context. Normally, it is called from API invoked from hand-written code, but also from TemporaryBuffer for low level maintenance. In the latter case the parameter is null and the method will quick-out. If it cannot uniquely identify the context, a message will be logged. Maybe this should be more aggressive since if a collision is detected the custom SQL will fail to execute correctly, so the customer should update it. However, since all known hand-written code is ST, this is not yet the case.

RecordBuffer.updateTenant: not something to address right now, but I am a little concerned about the performance implications of cycling through all active buffers when the tenant changes. Perhaps an alternative implementation would be to maintain a counter of tenant changes in the Persistence object and in the buffers, and only if the buffer's counter is lower than that of the Persistence object, update the buffer's state. Not ideal either, since it would require checks where the context is used within the buffer. So, let's put a pin in this until we can profile and figure out whether this actually represents a bottleneck.

OK. As noted above, this is necessary, and probably will fix #9362. If we can somehow optimize it, that would be great.

RemoteIdentityManager.setPersistence and RemotePersistence.createLockManager: why is Persistence.META_CTX used in these cases (also in the sequence handlers' safeContains methods)?

META_CTX is just a marker. These places are not affected by MT (the meta database is shared for entire database).

getTenantType methods in P2OAccessWorker and P2OLookup need javadoc.

Adding them. Thank you for spotting them.

Committed revision 15600 of 9057a. It contains the changes noted in above discussion (review).

I think the compiling is quite incremental. Even I get past #9057-42, new issues are present:

• persistence.executeSQL(batchStatsUpdate, new Object[] { primaryKey.getValue() }); (incompatible types: Object[] cannot be converted to boolean)
• LockManager manager = persistence.getLockManager(); (error: method getLockManager in class Persistence cannot be applied to given types;)
• this.database = persistence.getDatabase(); (error: method getDatabase in class Persistence cannot be applied to given types;)

I am not sure if these are the last ones ... the compilation halts after these errors are printed (the same happened for the last iterations).

Alex, if you have time before Ovidiu gets in, please add back the missing APIs upon which the ChUI application is dependent, in order to get through this last bit of regression testing. Please follow the same model Ovidiu used with the other ones he recently added back. Commit these to 9057a. I'd really like to get this merged to trunk today, assuming it passes.

I added those 3 methods reported in note 50. Committed revision 15601.

Note that all methods which were added having NOTE: This method is not multi-tenant compatible. are only used for testing, the new applications should NOT use the APIs.

Ovidiu, compilation worked (no more errors); I am moving on with run-time testing.

Alexandru Lungu wrote:

Ovidiu, compilation worked (no more errors); I am moving on with run-time testing.

Where does this testing stand?

I have committed a small clean revision and rebased the branch to 15584.
I pushed up to revision 15609. If the last regression test pass, the branch is ready to be merge in.

Where does this testing stand?

The regression testing passed.

Ovidiu, looking back over #9057-48, when we were discussing the use of the META_CTX constant w.r.t. sequence handlers, you noted:

META_CTX is just a marker. These places are not affected by MT (the meta database is shared for entire database).

I'm reading this to mean that all application schema-level sequences (by that I mean all sequences except p2j_id_generator_sequence) exist only at the shared database. We've since learned that sequences can be tenant-specific, the same as tables.

If my understanding that this is not already supported is correct, could you please consider the level of effort needed to implement this additional functionality (i.e., DDL generation, conversion of DMO interfaces, runtime support, etc.)? Thanks.

Indeed, we are not handling the sequences the right way. At this moment, all of them are processed within the default database. Apparently, this is not correct. They can be tenant private. The p2j_id_generator_sequence is a particular instance, which should be treated in the same way as meta tables. In fact this is what it is. Now that I think about this, it's clear that the PKs of the records from different tenant databases do not overlap. This means we need to take care that this will also not happen when the tenant databases are imported. My first solution of simply import data from each tenant individually into respective database will not work correctly with resect to PK unicity. But this is a subject for another task.

Regarding the regular sequences, now if I look at the sequence primitives, the 4GL offers (dynamic) functions which also have the tenant as parameter. I understand that these forms are only used by super-tenants, but the simple fact that they exist should have given me the confirmation the sequences can be tenant private.

At conversion, we currently create an enum (named _Sequences) which contains all necessary data for sequence management. The @Sequence annotation must be enhanced to contain the tenant type. The adjustment should not be a very complicated simple thing. I do not expect any change regarding the DDL.

At runtime, the above information should be read by the sequence manager and when a primitive (current / next / set) is executed, the proper context should be selected. The forms with tenant parameter are probably more difficult to implement, but I think we can defer to a later time, when we will implement the support for super-tenants. I think the main part of the implementation is there (that's the selection of the right database based on sharedDb flag I implemented in 9057a). The tenant-specific error conditions will have to be also to be tested and support added.

I have resumed work on the sequence issues. I have created a couple of testcases and I am now putting them to test in 4GL environment to have a base target for what FWD should do.
Because a dedicated task was created (#9503), I will continue reporting the advancements there.