FWD » Core Development » Database

At the moment, I am thinking of overriding the equals in BaseRecord and make it check the elements in data array. I think this is right based on the invariant posted by Alex.

I found this note in CompareOps.equals: // special case: an empty comhandle is considered be equal to the zero integer, so will take this into account.

LE: I actually don't think this is an issue, as we should have the same type on the same position in the data arrays, based on the invariant.

At the moment, I am thinking of overriding the equals in BaseRecord and make it check the elements in data array. I think this is right based on the invariant posted by Alex.

This is risky. I think there are place in FWD that use equals (from Object) for that matter and they rely on == comparison. Overriding equals may break such cases:

• Please investigate if there are equals comparisons of BaseRecord across FWD.

Alexandru Lungu wrote:

• Please investigate if there are equals comparisons of BaseRecord across FWD.

I actually found just one usage of Record.equals in LocalSyncher.preSynch: if (foreignDMO null || foreignDMO.equals(newForeignDMO)). We can easily replace this with == comparasion.

Got it, please go ahead.

Also, double-check this. It is crucial to ensure equals was not used somewhere else already for comparing BaseRecord.

equals is used explicitly only there, however I just realised it could be used as key in collections like Set or HashMaps, therefore equals will be called. And indeed, at a quick search, it is used in Set s multiple times.

So, I think I will abandon the idea to override equals, and, simply create another method which will do this checking.

Alexandru Lungu wrote:

Assert as in assert statement from Java. If so, did you run the Java process with -ea to activate it?

Yes, I modified server.sh with that option. First, I just used an assert which always failed to be sure it was working as expected :)

Personally, I rather use debug mode and use a breakpoint for that matter.

Yeah, probably would have been easier.

Assert as in assert statement from Java. If so, did you run the Java process with -ea to activate it? Personally, I rather use debug mode and use a breakpoint for that matter.

These must be removed. We never use Java assert. It has the potential to cause crashes in production depending on command line options which don't control. That is not OK.

I just used the assert to validate that I get the same results with the new implementation vs the old one while running some tests of a large customer application. I deleted the assert before committing.

OK

Ovidiu, please take a look over the changes in 9701a when you are available.

Andrei, please do other remaining testing until Ovidiu provides a final Review (unit tests or/and ChUi regression tests). This should reach trunk in the next couple of hours.

• unit tests for a large customer application [ai]
• fwd tests of another large customer application [me]

The unit tests of a customer passed, however I have one more failing test in the fwd tests. So, I am running the fwd tests again.

I have ran the fwd tests and they show no regressions.

Thanks, Ovidiu, for the review.

• unit tests of a large customer application [me]
• harness tests of another client [ls]

Ovidiu/Alex, do you think anything else is necessary?

I think that should suffice. The changes are right at the heart of the persistence and surely the affected code will be executed with a lot of data.

Andrei Plugaru wrote:

Alexandru Lungu wrote:

• Please investigate if there are equals comparisons of BaseRecord across FWD.

I actually found just one usage of Record.equals in LocalSyncher.preSynch: if (foreignDMO null || foreignDMO.equals(newForeignDMO)). We can easily replace this with == comparasion.

Please note that LocalSyncher is effectively dead code. We really should remove its use. It is based on the notion that we can have foreign keys between primary tables. We have long since determined that this is not feasible, since there is no referential integrity enforced by the Progress database, so much application code is written that would break referential integrity. This was causing database errors when applied to modern DBMS', so we stopped using it.

Persistence.isForeignKeysEnabled really should return false, or be removed entirely (along with code that depends on it returning true).

Eric Faulhaber wrote:

Please note that LocalSyncher is effectively dead code. We really should remove its use. It is based on the notion that we can have foreign keys between primary tables. We have long since determined that this is not feasible, since there is no referential integrity enforced by the Progress database, so much application code is written that would break referential integrity. This was causing database errors when applied to modern DBMS', so we stopped using it.

In the end, actually no changes to LocalSyncher were necessary for this task, but I see your point that we have to refactor to remove that parts of code.

If we are not in a rush for 9701a to be merged, we can do this here, if not, defer to another task.

As the proposed changes are not really related to this task, I created a new one - #9714, which will address the removal of LocalSyncher and the other necessary changes.

I will move on to the testing proposed in #9701-22.

All the testing showed no regressions. ✅

I think this can be queued in for merge.

I have merged to trunk as rev 15750.

Danut, should I also port it to 7156c and 7156e?

Andrei Plugaru wrote:

I have merged to trunk as rev 15750.

Danut, should I also port it to 7156c and 7156e?

Port the change to both branches (7156c and 7156e). See #7156-457 for reference.