FWD » Core Development » Database

Does this mean that existing imported databases may have some indexes which need to be dropped?

I am not sure I fully understand the question.

TL;DR: If the import decides that a table is processed, the indexes will be recreated.

If a database (regardless of its type: tenant or default) is re-imported, the tables which already have data will not be touched. Including the index set. For all other tables, the indexes will be re-set. Either after importing the data (as we previously were doing) or as 'empty' table. If the indexes for a table were set previously (because of an 'empty' import or because the application deleted all records), a new import will reindex the table (this was also a part of the previous implementation where the unique indexes were preserved during the data import to be sure there are no unique key conflicts, and the other indexes were created after the import to avoid incremental index updates).

The algorithm goes like this:

let ts = set of active and empty tables from current database
for each t in ts:
    drop all indexes of t
    create unique indexes
    if the .d file for the table was found:
         create records
    create non-unique indexes

The active above means the file is accessible by 4GL code according to tenant rules. For example a pure tenant table is not visible in default/shared database, so it is inactive.

I'm trying to determine if we need to write a migration script to cleanup existing MT installations so that they match the schema that would be present after 10094a.

In other words, is this a "breaking change"?

What 10094a adds is that the empty tables from the (re-)imported database are guaranteed to be indexed.

Do you think of something specific that can be broken?

• if the table is still empty, the indexes will be re-created;
• possible some data with duplicated unique keys in those indices. Running a 10094a import will ignore the tables. Attempting to do so will just raise an error, but the table remains corrupted.
• some data exists, but there are no index key duplication. This would be a ticking bomb for the situation above.

10094a works optimally by assumes the database was freshly created or the previous data was correctly imported. If we want a sanity check we must implement a re-indexing utility which should drop and recreate all indexes in all tables, with no assumption of the correctness of the existing data. Index key collision may be detected and the admin(?) should be prepared to handle the situation. This can be done simply by executing the appropriate ddl/schema_index_<table>_<dialect>.sql script. They are generated to contain both the drop of the indexes and their creation. In fact this is the reason of their existence. The import does not use them, instead the indexes are generated on-the-fly from meta information from @Table annotation of each DMO interface.

Please let me know if I got your question right.

Please let me know if I got your question right.

Yes, mostly.

If the schema can be incorrect from prior runs of the import as compared with what would exist by importing using 10094a, then we need to explain to the customer how to fix that when migrating to that version. If running ddl/schema_index_<table>_<dialect>.sql script is what is needed, then let's make sure they know.

We will need to have tested that as well to confirm that it resolves everything safely.

The ideal test 10094a would be to have a logical database with the shared db at least two tenant dbs. The customer did not provide any tenant database in dump (.d) format; instead we only have a PSQL dump. I will do the tests on one of our customers, using db fragments, partially importing with old revisions and finishing with 10094a. But this will not cover tenant specific code.

I will also do the full re-index test. Maybe drop some unique indexes and manually alter the data so that the collisions will occur when unique index will be created. The script will probably stop at that point and the situation will have to be manually handled since we do not have a protocol for automatically solving these cases.

Greg Shah wrote:

We will need to have tested that as well to confirm that it resolves everything safely.

As a first test I wrote the ant target and re-indexed the hotel database. I queried the set of indexes before and after, and the result was unaffected.

• incorrect permit to use duplicate keys for where unique indexes would not allow it;
• slow access to data since the query navigation will be done sequentially instead of indexed.

Most likely, the above issues were not yet observed since the tables were empty and if records are added at runtime they are just a few in our tests so the performance was not a problem and the unique index constraints were not broken.

Among those 90+ tables, some .d files were missing, some had size 0 and the import was skipping them. I will do a final commit and put it back to test.

Conclusion: beside the new database which will be imported with 10094a+, we will have to run the new ant reindex.db (or similar if ant is not available) on all installed database instances!

I made sure the tables are not skipped if the .d files are empty. Replacing all System.out with proper logging.
Branch 10094a was updated and rebased to trunk 15978. The current revision is 15981.

I upgraded the project of the single customer to be MT-enabled. Committed to r89.
Here are the changes in build.properties:

• The declaration of logical databases remain the same. They are also the name of the schema (the private and shared databases share the same schema, aka .p2o file):
  

  db.names=db1,db2

• For MT databases, there are two entries: one which declares the physical shared database name and one with the list of physical tenant database names:

  # MT mode (preferred, if exists):
  db.shared.db1=db1_shared
  db.tenants.db1=db1_tenant1,db1_tenant2

  Note that the SQL name of these databases and the name of the folders which contain the dump for the respective databases must match.

• In case of a ST database, the syntax is kept:
  

  # ST mode (default, if no MT defined):
  db.sql.db2=sql_db2

• The old user/password combination for normal user and admin are kept, but as default values in case a more specific combination was not specified:

  #default user/password combinations
  sql.user=<fwd-user>
  sql.user.pass=<fwd-user's-password>
  sql.admin=<adm-user>
  sql.admin.pass=<adm-user's-password>

• Our customer has different user/password combinations for each physical database (a normal thing, otherwise), so the above we expanded to accommodate combinations for each physical database (regardless of its tenant type):

  # specific user/password combinations for [db1_shared] database
  sql.user.db1_shared=<fwd-user-1.0>
  sql.user.pass.db1_shared=<fwd-user's-password-1.0>
  sql.admin.db1_shared=<adm-user-1.0>
  sql.admin.pass.db1_shared=<adm-user's-password-1.0>
   
  # specific user/password combinations for [db1_tenant1] database
  sql.user.db1_tenant1=<fwd-user-1.1>
  sql.user.pass.db1_tenant1=<fwd-user's-password-1.1>
  sql.admin.db1_tenant1=<adm-user-1.1>
  sql.admin.pass.db1_tenant1=<adm-user's-password-1.1>

  In our case, there is no specific definitions for the 2nd tenant database, so it will inherit the default user/password combinations.

Code review 10094a/15985-15988:

ImportWorker$TenantIdContext.prepareNextBracket: Why is it safe to fall back to using the superclass' implementation upon catching an SQLException? That seems like it should be a fatal error, since we no longer have an assurance of non-overlapping PKs.

for each .d file, if records is present in the PSC footer, that will be the number of optimally pre-allocated PKs;

Maybe I missed it; is this implemented? It looks like ID_BRACKET_SIZE is used unconditionally. I think it's ok if not, just making sure I didn't overlook it somewhere.

I think the rest of the update is good.

The changes are pervasive enough that, as discussed in the standup, we'll need not only to test in MT mode, but do serious regression testing for non-MT mode.

One other thing I noticed in my last review, though it already existed in the code before your recent changes. In ImportWorker.runImport, we check that the SQL state reported in a SQLException is not "42P01" (table not found error).

Is this SQL state common across dialects, or is it specific to PostgreSQL? If the latter, it should be managed by the Dialect infrastructure and it suggests we are missing something important here for other dialects. If the former, let's document that fact in the code, so it is clear to future readers.

Eric Faulhaber wrote:

ImportWorker$TenantIdContext.prepareNextBracket: Why is it safe to fall back to using the superclass' implementation upon catching an SQLException? That seems like it should be a fatal error, since we no longer have an assurance of non-overlapping PKs.

OK, I am changed it to re-throw the SQL exception wrapped in a runtime exception.
My idea was to handle this as a WARNING, and allow the process to continue after informing the user about the exceptional situation. But it makes more sense to make it fatal.

for each .d file, if records is present in the PSC footer, that will be the number of optimally pre-allocated PKs;

Maybe I missed it; is this implemented? It looks like ID_BRACKET_SIZE is used unconditionally. I think it's ok if not, just making sure I didn't overlook it somewhere.

• we encountered numerous cases when the information is not available
• pre-fetching for small- medium tables is not a problem, but if the import fails for a large table, a corresponding number of PKs will be lost

So I implemented it to use the fetched PKs across table, leaking the PS to next one in the same worker. The only issue here is to find the optimum value for ID_BRACKET_SIZE: not too large - to avoid dropping them at the end of process, and not too small - to avoid requesting them for the server. Maybe it would be a good idea to make this a parameter instead of have it hardcoded. From my experience with the new implementation, the success notification for prefetch is a bit aggressive, even with INFO level.

The changes are pervasive enough that, as discussed in the standup, we'll need not only to test in MT mode, but do serious regression testing for non-MT mode.

I am planning a set of imports for larger databases of the ST projects.

One other thing I noticed in my last review, though it already existed in the code before your recent changes. In ImportWorker.runImport, we check that the SQL state reported in a SQLException is not "42P01" (table not found error).
Is this SQL state common across dialects, or is it specific to PostgreSQL? If the latter, it should be managed by the Dialect infrastructure and it suggests we are missing something important here for other dialects. If the former, let's document that fact in the code, so it is clear to future readers.

It was PSQL specific. I added specific tests for all other dialects.

Committed revision 15989.

Eric/Constantin: Please review this ASAP so we can move this task ahead.

Constantin Asofiei wrote:

Ovidiu, are there any changes required in existing scripts (non-tenant and multi-tenant) for data import?

Only for MT, the import process requires 3 additional parameters: tenantDefaultDbUrl, tenantDefaultDbUser and tenantDefaultDbPassword. See note #10094-3. They are set using initializeTenant() method in ImportWorker.
The database structure needs to be specified in the build.properties. See note #10094-14.

• in case of MT, the PKs are taken from the shared database (that's why the above additional parameters are needed). They are pre-fetched in ID_BRACKET_SIZE buckets and used incrementally within an import thread until depletion, when a new bucket is requested from shared database;
• import.xml was cleaned up a bit of dead code;
• improved support for importing empty tables. The old "empty" mode is gone, now it is implicit meaning the indexes are configured for all tables, including for those which have no .d files to import from or if the file has no data;
• ImportWorker no longer use System.out / System.err for logging.

For the only customer which uses MT, I updated the build.properties, build.xml, build_db.xml from the project. The current revision is 89. The other projects should not be affected.

Branch 10094a was rebased to trunk 16032. It is currently at r16039 and ready for review.

TenantIdContext.prepareNextBracket - there is a while (rs.next()) which suggets that there may be more than one row returned by the prefetch SQL. Please change this to an IF. But actually, I don't understand the entire code:

                  while (rs.next())
                  {
                     if (restInBucket++ == 0)
                     {
                        currentId = rs.getLong(1);
                     }
                  }

Why the if (restInBucket++ 0) 0? Just to get the first result?

Constantin Asofiei wrote:

Why the if (restInBucket++ == 0)? Just to get the first result?

Sorry for the C/C++ programming style. I should have written as:

                  while (rs.next())
                  {
                     if (restInBucket == 0)
                     {
                        currentId = rs.getLong(1);
                     }
                     restInBucket++;
                  }

The invariant at the end of the loop is that currentId represents the first free PK to be used (if restInBucket is not 0) while restInBucket represents the number of free PKs fetched in by the request. The loop is necessary since there is no other way to detect (in a dialect independent manner) the actual number of rows in a query except by fully iterating it.

I used branch 10094a to test the import process on a large ST database and performed a smoke-test on it. No problems surfaced.
An additional test was performed on hotel_gui. No problems also.

Currently, I am unable to re-run the ST/MT imports on the only MT-enabled project because the DMO are not matching. I understand tomorrow there will be a new conversion available, so I will try again then.

The customer specific documentation on database import was updated to reflect the changes in this task. Specially, a 7th section was added at the end of the the page which show the expected shell command needed to be scripted.

BTW:
Branch 10094a was rebased to trunk 16055. Current revision is 16062.

Code review 10094a/16060-16062:

Looks good overall.

The only objection I have is that the dialect-specific error handling at ImportWorker:974 should be managed through the Dialect class hierarchy, not inline. We already have a dialect instance variable in the ImportWorker instance; let's use it here. Leaving it inline means technical debt: it leaves one more location in the code where we have to remember to make a dialect-specific update when adding a new dialect implementation.

Eric,

Yesterday I've updated the branch 10094a according to suggestions in the review from #note-26. It was also rebased to a newer trunk revision.

There was a regression reported in #7143-1580 (most likely during the merge operation). This branch would fix it. I think we should merge this ASAP, so I will (re-)do a final set of tests for MT project (psql), and ST (H2, PSQL, MariaDb, MSSQL). Thanks.

Ovidiu Maxiniuc wrote:

10094a was rebased. Additionally, it contains the fix for set sequence value for PGSQL dialect, as discussed in standup.
Please review.

Ovidiu- We've recently started seeing this error in our <report app> logs. Could it be related to what you have fixed here? <abc>_seqno is actually a sequence not a column.

25/07/22 13:26:31.552Z | SEVERE | com.goldencode.p2j.persist.Persistence | ThreadName:Conversation [000000E5:rptsch], Session:00000229, ThreadId:00000033, User:rptsch | Error executing SQL statement: select setval(<abc>_seqno, 1)
com.goldencode.p2j.persist.PersistenceException: Failed to execute [select setval(<abc>_seqno, 1)]
Caused by: org.postgresql.util.PSQLException: ERROR: column "<abc>_seqno" does not exist

Carson, you are right. this is the exact issue the last commit will fix. It is a regression introduced in the last week. We noticed it in the last 24h, but luckily, it is a quick fix.
I injected it to this branch to quickly reach the trunk. Waiting input/review from Eric.

Thank you Ovidiu for the update and fix. We were going to report separately.

The only untested part is the very last commit (related to setval for sequences). It passed my local tests, but I have not regression-test it with any customer application. I will put it to the test with the MT application and the unit-tests for another ST app. Although I am not sure if these will cover the set-value 4GL statement. Will report back when finished.

Both aforementioned regression tests passed.

Ovidiu/Alexandru, 10094a needs to be merged today, if we plan to push to xfer.

The erroneous output of the _seqno to log files fixed against this RedMine and reported against https://proj.goldencode.com/issues/10335 is no longer occurring.